Check the size of `build_path` #653

nkaretnikov · 2023-11-05T22:31:04Z

Fixes #649.

netlify · 2023-11-05T22:31:09Z

✅ Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name	Link
🔨 Latest commit	`e2a7618`
🔍 Latest deploy log	https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65511ef73490960008ffc55d

nkaretnikov · 2023-11-05T22:31:30Z

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


Not used in build_directory.

How come? Not anymore? I thought it had the hash, timestamp and name? Or is that part of the 'build_key' thingy?

It was not used before this PR, just a mistake that I fixed since I was making changes here anyway.

This is the one used below, no name here:

build_directory = Unicode( "{store_directory}/{namespace}", help="Template used to form the directory for storing conda environment builds. Available keys: store_directory, namespace, name. The default will put all built environments in the same namespace within the same directory.", config=True, )

I thought it had the hash, timestamp and name? Or is that part of the 'build_key' thingy?

Yes, it's in build_key, which we append later:

def build_key(self): ... return f"{self.specification.sha256}-{self.scheduled_on.strftime(datetime_format)}-{self.id}-{self.specification.name}"

nkaretnikov · 2023-11-05T22:33:11Z

TODO:

One of the CI integration tests fails. I think this might be this check, due to CI having a longer conda-store state dir location.
I think this needs a unit test, to make sure the error is raised.
A simple integration test would be trying to create an environment with a very long environment name. This affects the build_key and hence the build_path/prefix, so should error out.
- I also want to check the effect of this on the UI. Will it throw 500 with an error printed to the console? Can we show a friendlier error message in the UI?
- Related bug: [BUG] - Raise an error in the UI if env name is too long #654
- By finding the number of 'a's that would not exceed the filename limit, it's possible to hit the error I'm adding in this PR, but as with the above bug, it's currently not shown in the UI
- Should be fixed. Unfortunately, there's no way to communicate an error message right now because it's only appended to the logs, since schema.BuildStatus.FAILED doesn't contain an error message. I'll see if this can be added easily.
See if it's possible to report more info with status FAILED.

Fixes conda-incubator#649.

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 conda-incubator#654.

conda-store-server/tests/test_server.py

These tests check whether the status is set to FAILED when an exception occurs in a task.

This is an additional way to communicate information about the status of a build, which will be exposed to the user.

nkaretnikov · 2023-11-07T13:28:43Z

This PR should be merged first since it has new tests, which are useful. I think it will conflict with #652 because it uses a migration (migration hashes won't match). #531 might require rebasing as well.

Companion PR for conda-incubator/conda-store#653

nkaretnikov · 2023-11-07T19:55:03Z

Companion PR on the UI side (see screenshot there): conda-incubator/conda-store-ui#332

nkaretnikov · 2023-11-07T21:15:21Z

conda-store-server/tests/test_server.py

+        # OSError: [Errno 36] File name too long
+        255,
+        # OSError: [Errno 36] File name too long
+        256,


These error messages the code used to throw before I moved build_path to the start of the function in build_conda_environment. Note how the messages here are different compared to tests/test_api.py, this is correct since different backends are used.

nkaretnikov · 2023-11-07T21:22:19Z

tests/test_api.py

+    )
+    if size > 255:
+        assert response.status_code == 500
+        return  # error, nothing to do


Note the change here compared to test_server.py. Again, this is due to different backends being used for these.

In the UI this manifests as follows: I attempt to add an env with 256 'a's, click on the "Create" button in "Create Environment", but nothing is shown. The env is also not created. This is something I'll need to fix in the companion UI PR - the UI should show an error.

Companion PR for conda-incubator/conda-store#653

jaimergp

Looks good!

The main part of this PR is:

if len(str(res)) > 255:
    raise BuildPathError("build_path too long: must be <= 255 characters")

I think we can afford to be more detailed here, but I don't know if that's the correct place. Maybe just codifying the error type with an identifier+number we can add some documentation on how to work around this issue?

jaimergp · 2023-11-10T13:23:58Z

conda-store-server/conda_store_server/utils.py

@@ -15,6 +15,10 @@ def message(self):
        return self.args[0]


+class BuildPathError(CondaStoreError):


I'm more used to having exceptions in an exceptions.py module, but I guess there's a precedence to have them in utils.py? Probably worth its own issue, not blocking here.

Yeah, agree. But not changing it now. Added it next to the existing exception.

Let's add an issue to cover this later.

conda-store-server/tests/test_server.py

tests/test_api.py

conda-store-server/conda_store_server/build.py

conda-store-server/conda_store_server/orm.py

jaimergp · 2023-11-10T13:40:48Z

conda-store-server/conda_store_server/orm.py

+        # 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 characters")


Maybe we can afford to be more informative here and report which components are responsible for build_path. What about:

f"'build_path' is too long ({len(str(build_path))}). Maximum allowed length is 255. See docs at xxx for more information.

This is also an opportunity to start codifying error numbers and have a reference online so folks can troubleshoot, à la https://flake8.pycqa.org/en/latest/user/error-codes.html. (worth its own issue).

That what I had originally, but I removed it. See here #653 (comment). As for codifying the errors: not adding that here either. There's no policy/format on this right now. Similarly, saying "see the docs" is not helpful. This also needs to be linked and done in a systematic way, otherwise links will point to nowhere over time. Not adding this also because there's no infra for this right now. Let's keep the PR self-contained, we can always change this later.

Let's keep the PR self-contained, we can always change this later.

Works for me, but let's make sure there's an issue we can iterate on.

jaimergp · 2023-11-10T13:42:19Z

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


How come? Not anymore? I thought it had the hash, timestamp and name? Or is that part of the 'build_key' thingy?

nkaretnikov · 2023-11-12T17:02:30Z

I think we can afford to be more detailed here, but I don't know if that's the correct place. Maybe just codifying the error type with an identifier+number we can add some documentation on how to work around this issue?

I initially printed the whole path and its size, too. But that'd be flaky on CI because different directories are used everywhere. It also seems like an implementation detail, which is not helpful unless you have admin rights. I'll just add documentation for now and we can improve later.

nkaretnikov · 2023-11-12T17:54:05Z

Hey @jaimergp, thanks for your comments! I'm going to document the build_path in docs/administration.md, similar to how we document the MAX_PATH issue on Windows. Otherwise, I don't think there's a need to change anything else.

UPD: Added documentation.

jaimergp

The PR covers enough to have some protection against too long paths. The information conveyed could be more detailed, and less hardcoded. But that can be worked on in future PRs.

For example, I could imagine the UI receiving a JSON payload with the details of the error instead of a simple string. And then we decide what to do with the payload depending on the user role

At the same time, we've identified an opportunity to standardize error messages with error codes for easier (and more accurate) documentation.

These two improvements could help in future efforts like e.g. localization or theming.

Issues need to be created to cover these suggestions, but fwiw, this PR is good to go on my side.

trallard · 2023-11-15T17:42:44Z

@jaimergp is this ready for merge? if so then perhaps you and/or @nkaretnikov should open a follow-up issue with potential improvements or areas to explore and merge this straight away

nkaretnikov commented Nov 5, 2023

View reviewed changes

nkaretnikov force-pushed the prefix-size-check-649 branch from 71dd8a5 to e69c999 Compare November 6, 2023 16:32

Check the size of build_path

291f12a

Fixes conda-incubator#649.

nkaretnikov force-pushed the prefix-size-check-649 branch from e69c999 to 291f12a Compare November 6, 2023 19:06

Set status to FAILED in build_conda_environment

4df1c2a

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 conda-incubator#654.

nkaretnikov commented Nov 6, 2023

View reviewed changes

conda-store-server/tests/test_server.py Outdated Show resolved Hide resolved

Test build status on failure

34ebf5b

These tests check whether the status is set to FAILED when an exception occurs in a task.

nkaretnikov force-pushed the prefix-size-check-649 branch from 26a441b to 34ebf5b Compare November 6, 2023 22:06

Add status_info

0a28811

This is an additional way to communicate information about the status of a build, which will be exposed to the user.

Nikita Karetnikov added 2 commits November 7, 2023 16:12

Fix build_path test to account for recent changes

63aeb15

Update error message

34ac2dd

nkaretnikov force-pushed the prefix-size-check-649 branch from 1ff7efe to 34ac2dd Compare November 7, 2023 18:51

nkaretnikov pushed a commit to nkaretnikov/conda-store-ui that referenced this pull request Nov 7, 2023

Expose status_info in the UI

1b4ad19

Companion PR for conda-incubator/conda-store#653

nkaretnikov mentioned this pull request Nov 7, 2023

Expose status_info in the UI conda-incubator/conda-store-ui#332

Merged

nkaretnikov commented Nov 7, 2023

View reviewed changes

nkaretnikov marked this pull request as ready for review November 7, 2023 21:59

nkaretnikov requested a review from jaimergp November 7, 2023 21:59

nkaretnikov pushed a commit to nkaretnikov/conda-store-ui that referenced this pull request Nov 8, 2023

Expose status_info in the UI

69d3673

Companion PR for conda-incubator/conda-store#653

jaimergp reviewed Nov 10, 2023

View reviewed changes

Document build path length limits

e2a7618

nkaretnikov requested a review from jaimergp November 12, 2023 19:00

This was referenced Nov 12, 2023

[ENH] - Ensure that the length of all environment paths <= 255 characters #649

Closed

Use shorter build_key #652

Merged

jaimergp approved these changes Nov 14, 2023

View reviewed changes

trallard added type: enhancement 💅🏼 needs: review 👀 project: challenges labels Nov 14, 2023

nkaretnikov merged commit a5680f8 into conda-incubator:main Nov 16, 2023

This was referenced Nov 16, 2023

Unify user-facing error messages #663

Closed

[BUG] - unit-test conda-store-server (ubuntu) hangs on CI #666

Closed

nkaretnikov mentioned this pull request Dec 13, 2023

Figure out how to handle long paths on Windows #588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check the size of `build_path` #653

Check the size of `build_path` #653

nkaretnikov commented Nov 5, 2023

netlify bot commented Nov 5, 2023 •

edited

Loading

nkaretnikov Nov 5, 2023

jaimergp Nov 10, 2023

nkaretnikov Nov 12, 2023 •

edited

Loading

nkaretnikov commented Nov 5, 2023 •

edited

Loading

nkaretnikov commented Nov 7, 2023

nkaretnikov commented Nov 7, 2023

nkaretnikov Nov 7, 2023 •

edited

Loading

nkaretnikov Nov 7, 2023 •

edited

Loading

jaimergp left a comment

jaimergp Nov 10, 2023

nkaretnikov Nov 12, 2023

jaimergp Nov 14, 2023

jaimergp Nov 10, 2023

nkaretnikov Nov 12, 2023

jaimergp Nov 14, 2023

jaimergp Nov 10, 2023

nkaretnikov commented Nov 12, 2023

nkaretnikov commented Nov 12, 2023 •

edited

Loading

jaimergp left a comment

trallard commented Nov 15, 2023

		@@ -15,6 +15,10 @@ def message(self):
		return self.args[0]


		class BuildPathError(CondaStoreError):

Check the size of build_path #653

Check the size of build_path #653

Conversation

nkaretnikov commented Nov 5, 2023

netlify bot commented Nov 5, 2023 • edited Loading

✅ Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkaretnikov Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

nkaretnikov commented Nov 5, 2023 • edited Loading

nkaretnikov commented Nov 7, 2023

nkaretnikov commented Nov 7, 2023

nkaretnikov Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

nkaretnikov Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

jaimergp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkaretnikov commented Nov 12, 2023

nkaretnikov commented Nov 12, 2023 • edited Loading

jaimergp left a comment

Choose a reason for hiding this comment

trallard commented Nov 15, 2023

Check the size of `build_path` #653

Check the size of `build_path` #653

netlify bot commented Nov 5, 2023 •

edited

Loading

nkaretnikov Nov 12, 2023 •

edited

Loading

nkaretnikov commented Nov 5, 2023 •

edited

Loading

nkaretnikov Nov 7, 2023 •

edited

Loading

nkaretnikov Nov 7, 2023 •

edited

Loading

nkaretnikov commented Nov 12, 2023 •

edited

Loading