ARXIVNG-2210 ARXIVNG-2211 filemanager refactor #43

erickpeirson · 2019-06-05T20:24:59Z

The initial implementation of the file manager service focused on replicating the behavior of the legacy file checks, and so closely reproduced much of the logic and abstractions. This implementation was quite fast in local tests, correct, and had excellent test coverage over known problematic uploads.

Problem: EFS I/O is too slow to run checks

EFS I/O limits scale with overall volume size, which makes things difficult from the start. But even if we provision a high level of I/O ahead of time, there are too many low-level reads and writes to perform file checks on EFS directly. A modestly sized tarball could take around 12 seconds to unpack and check, which is not acceptable.

So we decided that we need to refactor the application to perform checks on a local disk (e.g. in memory), and then move files over to EFS at the end of the request. This way we are keeping I/O to a minimum.

Problem: the core checking and file-shuffling routines were too complex

Because the initial implementation stuck closely to the logic of the legacy system, the file management and checking logic had grown to nearly 3,000 lines largely contained in a single class, with really long methods doing the actual checking. This made it hard to understand what checks were being performed, and make changes that were predictable.

Since we were going to refactor to address the I/O issues, we decided to go ahead and perform an initial refactor for clarity, composability, and extensibility.

Refactor

Here is an overview of the refactor in 2019-06:

Abstracted away the underlying filesystem logic by encapsulating it in
adapters. A SimpleStorageAdapter implements logic that is close to the original implementation (on a single volume). A QuarantineStorageAdapter implements the two-volume logic needed to do fast checks before shipping data to EFS. The storage adapters implement an API/protocol that is formalized in the domain.
Separated the file/workspace checks from the upload workspace class itself using multiple-dispatch (something like the visitor pattern). The advantage of this pattern is that we can add more checks, reorder the checks, etc, without creating an even more enormous class. We can also test them separately, which we should start to do...
- Checkers (visitors) are formalized in the domain, and extend a BaseChecker class.
- Checking strategies are also formalized in the domain, and implement the visitation of the checkers on a workspace. The SynchronousCheckingStrategy
  is the first such implementation.
- The UploadWorkspace class is assigned a set of checks and a strategy, which are applied via the UploadWorkspace.perform_checks method.
For clarity sake, decomposed most of the UploadWorkspace into mixins that add sets of
functionality.
Consolidated the storage of metadata about the workspace.
- Previously, details about the workspace state, and about the files in the workspace, were generated on each request (indeed, all or most checks were re-run on each request). In addition, a database row was maintained with some metdata about the last run through the checks.
- In the reimplemented checks, file-level checks are only run once per file, and workspace-level checks are only run after files are added or deleted.
- In the new implementation, the database service has the primary responsibility for loading metadata about the workspace, and initializing the UploadWorkspace with its storage adapter. This means that loading the workspace requires only a single function call, and there is no need to keep track of both a filesystem-based workspace object and a database object (both of which represent the workspace) in the controllers.
Since the core classes were significantly altered, much of the work involved modifying the extensive test suite to use the new internal APIs, and modifying the controllers to leverage the new patterns in the domain and processes. Since this involved going through the test suite one case at a time, I took the liberty of breaking things up into more manageable pieces:
- Split controllers out into submodules, for easier navigation.
- Split up super long test routines in the API tests into separate modules in
  (tests/test_api/), smaller TestCases, and smaller test_
  methods. Hopefully this makes it easier to find relevant tests and also see
  what specifically is being tested.
Removed some cruft that wasn't being used and wasn't likely to be used
(async boilerplate, etc).

…estore checkpoints.

…avid

…ing. [ARXIVNG-2032] David

… checkpoint tests.

…space.

DavidLFielding · 2019-08-11T18:47:23Z

filemanager/domain/uploaded_file.py

+        return self.path
+
+    @property
+    def name_sans_ext(self) -> str:


Is the same as (os.path.) basename? To be consistent should delete_file() become directory_sans_file()? The method name definitely caught my attention. I'm sure this is just fine.

Slightly different from basename:

>>> os.path.basename('foo/bar.txt') 'bar.txt' >>> os.path.splitext('foo/bar.txt')[0] 'foo/bar'

To be consistent should delete_file() become directory_sans_file()?

Not sure what you mean -- this is just generating a string based on the path. delete_file() has side-effects (namely, deleting a file)

I was really just poking fun at the method name. I apologize.

haha no worries :-)

DavidLFielding · 2019-08-11T19:18:15Z

filemanager/domain/uploads/__init__.py

+
+  - :class:`.ReadinessWorkspace`, which adds semantics around the "readiness"
+    of the workspace for use in a submission.
+  - :class:`.SingleFileWorkspace`, which adds the concept of a "single file


We've discussed eventually allowing ancillary files with 'single-file' submissions. Does this implementation limit in any way the ability to provide a single-file submission with ancillary data files?

Does this implementation limit in any way the ability to provide a single-file submission with ancillary data files?

No, because .file_count excludes ancillary files, and .is_single_file_submission is just looking at .file_count.

DavidLFielding · 2019-08-11T19:50:59Z

filemanager/domain/uploads/__init__.py

+            raise RuntimeError('Storage adapter is not set')
+        self.storage.makedirs(self, self.source_path)
+        self.storage.makedirs(self, self.ancillary_path)
+        self.storage.makedirs(self, self.removed_path)


Do we need to create system directory to be consistent?

Well, source_path is based on base_path, which is where the system files live. So it should exist by extension of these being created. I guess if you wanted to store system files somewhere else; hadn't considered that scenario. Do you think we should support this?

Ahh. I saw new system directory and thought you were now sticking the system/checkpoint files there. My bad. I need to take another look at the code.

DavidLFielding · 2019-08-11T20:27:59Z

filemanager/domain/uploads/file_mutations.py

+            self.files.set(u_file.path, u_file)
+
+    @modifies_workspace()
+    def delete_workspace(self) -> bool:


This class contains methods that 'alter files' according to class docstring. The scope of this method seems a bit beyond just altering files. Maybe just inaccurate description or maybe it's in the wrong place.

Agree it was misleading; fixed in 3d5ca4b

DavidLFielding · 2019-08-12T20:59:27Z

filemanager/process/check/file_type.py

+    return None
+
+
+# QUESTION: is this still relevant, given that FM service is decoupled from


At the current time if you provide the compilation service with a submission containing the missing font file AutoTeX basically aborts. So you could eliminate this check and let the compilation service generate an error. One could argue that many of the checks in the FM service are related to compilation and belong in the compilation service.

DavidLFielding · 2019-08-12T21:04:07Z

filemanager/process/check/missing_references.py

+logger = logging.getLogger(__name__)
+
+
+class CheckForMissingReferences(BaseChecker):


This no longer represents current logic in the Legacy system and needs to be updated.

DavidLFielding · 2019-08-12T21:09:40Z

filemanager/process/check/tex_format.py

+logger = logging.getLogger(__name__)
+
+
+# TODO: implement this.


Looks like you forgot to implement this functionality.

These weren't implemented before the refactor, IIRC

Right. That was a little wishful thinking on my part.

DavidLFielding · 2019-08-12T21:12:43Z

filemanager/process/check/tex_generated.py

@@ -0,0 +1,56 @@
+"""Check for TeX-generated files."""


I'm wondering if this needs to be reworded along the lines of 'eliminate files generated from TeX compilation' to distinguish it more from TeX Produced. TeXGenerated makes me think of TeXProduced.

fixed in 7545f3d

DavidLFielding · 2019-08-12T21:13:47Z

filemanager/process/check/top_level_directory.py

+                workspace.rename(child, new_path)
+            workspace.remove(entries[0], "Removed top level directory")
+            #
+            # # Set permissions


Permissions have always been an issue. I expect you are dealing with these elsewhere.

DavidLFielding · 2019-08-12T21:15:04Z

filemanager/process/check/unpack.py

+        workspace.create(dest, touch=False, is_ancillary=is_ancillary)
+
+
+# TODO: Add support for compressed files.


Will we accept Unix compressed files?

Dunno. This TODO was there before the refactor IIRC

filemanager/process/strategy.py

tests/test_api/_tests.py

DavidLFielding · 2019-08-12T21:34:01Z

tests/test_api/_tests.py

+#
+#
+#     # Upload a submission package and perform normal operations on upload
+#     def test_upload_files_normal(self) -> None:


These normal/typical upload tests were definitely working in the pre-refactored FM service. This test basically scripted a series of typical requests that might be expected during a normal submission process. The idea was to combine different types of requests together in order to catch any inconsistencies in the final workspace state.

Is this it?

arxiv-filemanager/tests/test_api/test_new_upload.py

Line 181 in 2f37ed6

# Upload a submission package and perform normal operations on upload

I think these just migrated elsewhere. I seem to recall running into them elsewhere.

tests/test_api/test_ancillary_upload.py

tests/test_api/test_checkpoints.py

tests/test_api/test_eps_repair.py

tests/test_api/test_existing_upload.py

DavidLFielding · 2019-08-12T22:15:18Z

tests/test_api/test_feature_missing_references.py

+logger.setLevel(int(os.environ.get('LOGLEVEL', '20')))
+
+
+class TestMissingReferences(TestCase):


These tests need revision as the bib/bbl logic has changed. (Ignore my comments on bib/bbl handling if you've already updated the internal logic per recent changes)

tests/test_api/test_new_upload.py

DavidLFielding · 2019-08-12T22:43:14Z

tests/test_api/test_quarantine.py

@@ -0,0 +1,344 @@
+"""Tests the application via its API, using the QuarantineStorageAdapter."""


This test appears to specifically test the new quarantine storage adaptor. Would it make sense to run all existing tests against each of the storage adaptor instances? Could you set the storage adaptor, run all of the existing tests, set a different storage adaptor, and run all of the tests again? My sense is if we are changing the underlying storage adaptors we should run an exhaustive series of tests that are not specifically tailored to the storage adaptors. Then again we probably need a few tests tailored to the storage adaptors. Just my two cents here.

tests/test_check_workspace.py

tests/test_api/test_upload_session.py

DavidLFielding

I managed to at least glance at most of the files in the PR. Hopefully, my comments are useful in some way. I may check a few more things if I have time but plan to move on to other tasks tomorrow.

At this point, I have not run the service beyond exercising the tests, which all passed.

DavidLFielding · 2019-08-13T21:05:12Z

I notice some formatting issues in my checkpoint PR so on a whim I decided to run some checks on File Manager.

pylint filemanager
...

Your code has been rated at 8.81/10 (previous run: 8.81/10, +0.00)

Yikes! I hear screams off in the distance.

There seem to be a huge number of trivial continue indentation and unused argument warnings.

Yet there are still a large number of errors that seem like they need to be addressed:

ex.
filemanager/domain/index.py:35: [E1137(unsupported-assignment-operation), FileIndex.set] 'self.system' does not support item assignment
...
filemanager/domain/index.py:47: [E1135(unsupported-membership-test), FileIndex.contains] Value 'self.system' doesn't support membership test
...
filemanager/domain/index.py:60: [E1136(unsubscriptable-object), FileIndex.get] Value 'self.system' is unsubscriptable

Lots of these...

filemanager/domain/uploads/stored.py:101: [W0613(unused-argument), IStorageAdapter.get_full_path] Unused argument 'workspace'
filemanager/domain/uploads/stored.py:102: [W0613(unused-argument), IStorageAdapter.get_full_path] Unused argument 'u_file'
...

filemanager/domain/uploads/file_mutations.py:384: [W0201(attribute-defined-outside-init), SourceLog.post_init] Attribute '_logger' defined outside init
filemanager/domain/uploads/file_mutations.py:385: [W0201(attribute-defined-outside-init), SourceLog.post_init] Attribute '_f_path' defined outside init
filemanager/domain/uploads/file_mutations.py:386: [W0201(attribute-defined-outside-init), SourceLog.post_init] Attribute 'file_handler' defined outside init

Here at arXiv we aim for a Pylint target score of 9/10. :^) wink wink

I realize you are extremely busy.

Would you like me to start eliminating the trivial sorts of Pylint errors? I'm not sure what's going on with so many unused argument errors and whether there is an easy global fix. Some of the other errors might be faster for you to resolve (if you think this is important).

DavidLFielding · 2019-08-13T22:13:36Z

While I'm at it I ran 'pydocstyle --convention=numpy --add-ignore=D401'

There are nearly 60 "Missing docstring" errors. Many of these are located in the newer file_mutations.py/storage.py code. Don't scream yet.

mypy generates a large number of errors. Most of these are trivial and easily correctable:

easy ones...lots of these...

filemanager/domain/tests/test_workspace.py:16: error: Function is missing a return type annotation
filemanager/domain/tests/test_workspace.py:16: note: Use "-> None" if function does not return a value

while some of these other errors might warrant a closer look:

filemanager/domain/uploads/stored.py:223: error: "IStorageAdapter" has no attribute "get_size_bytes"
filemanager/domain/uploads/stored.py:247: error: Returning Any from function declared to return "bytes"
filemanager/domain/uploads/translatable.py:77: error: "Type[datetime]" has no attribute "fromisoformat"
filemanager/domain/uploads/translatable.py:109: error: "TranslatableWorkspace" has no attribute "lastupload_readiness"
filemanager/domain/uploads/translatable.py:110: error: "TranslatableWorkspace" has no attribute "status"
...

along with several of the type mismatch flavor...

filemanager/domain/uploads/checkpoint.py:307: error: Argument 1 to "_update_from_checkpoint" of "CheckpointWorkspace" has incompatible type "TranslatableWorkspace"; expected "CheckpointWorkspace"
...

You may want to take a quick look to see if any of these need to be addressed.

Tomorrow I will fire up the submission UI suite and test the FM as I resume work on alpha tests.

erickpeirson · 2019-08-14T20:10:45Z

Would you like me to start eliminating the trivial sorts of Pylint errors? I'm not sure what's going on with so many unused argument errors and whether there is an easy global fix. Some of the other errors might be faster for you to resolve (if you think this is important).

Good catches. Some of these are really things that need to be fixed, and some are not.

For example, the unsupported-assignment-operation and other messages on those dicts are spurious, because pylint doesn't know that dataclasses is doing magic with field(...). So some of those can get # pylint: disable=ed. I also added the following to the .pylintrc as blacklisted:

unused-argument -- most of this comes from the use of protocols, and the standardized calling API of checker methods.
duplicate-code -- this is largely coming from tests. May be a better way to do this, not an expert on pylint config.
no-self-use -- this largely comes from the checkers; pylint doesn't know what patterns are being used here, so this is spurious.

erickpeirson · 2019-08-14T20:15:24Z

mypy generates a large number of errors. Most of these are trivial and easily correctable:

I'd recommend using ./tests/type-check.sh filemanager rather than running mypy directly (maybe you are already doing this). I had a tee in the wrong spot in that script, so it was printing ignored errors (the ones in the tests) even though ultimately those aren't counted against the final result. That being said, I am still seeing errors (which is somewhat disheartening, because I really thought that I had fixed all of these):

$ ./tests/type-check.sh filemanager
filemanager/domain/uploads/stored.py:225: error: "IStorageAdapter" has no attribute "get_size_bytes"
filemanager/domain/uploads/stored.py:249: error: Returning Any from function declared to return "bytes"
filemanager/domain/uploads/translatable.py:77: error: "Type[datetime]" has no attribute "fromisoformat"
filemanager/domain/uploads/translatable.py:109: error: "TranslatableWorkspace" has no attribute "lastupload_readiness"
filemanager/domain/uploads/translatable.py:110: error: "TranslatableWorkspace" has no attribute "status"
filemanager/domain/uploads/translatable.py:111: error: "TranslatableWorkspace" has no attribute "lock_state"
filemanager/domain/uploads/translatable.py:112: error: "TranslatableWorkspace" has no attribute "source_type"
filemanager/domain/uploads/translatable.py:141: error: "Type[TranslatableWorkspace]" has no attribute "Status"
filemanager/domain/uploads/translatable.py:142: error: "Type[TranslatableWorkspace]" has no attribute "LockState"
filemanager/domain/uploads/translatable.py:143: error: "Type[TranslatableWorkspace]" has no attribute "SourceType"
filemanager/domain/uploads/translatable.py:152: error: "Type[TranslatableWorkspace]" has no attribute "Readiness"
filemanager/domain/uploads/checkpoint.py:307: error: Argument 1 to "_update_from_checkpoint" of "CheckpointWorkspace" has incompatible type "TranslatableWorkspace"; expected "CheckpointWorkspace"
filemanager/process/strategy.py:34: error: Function is missing a return type annotation
filemanager/process/strategy.py:34: note: Use "-> None" if function does not return a value
filemanager/process/strategy.py:52: error: Need type annotation for 'tasks'
filemanager/process/strategy.py:56: error: Function is missing a return type annotation
filemanager/process/strategy.py:61: error: Function is missing a return type annotation
filemanager/process/strategy.py:61: note: Use "-> None" if function does not return a value
filemanager/process/strategy.py:74: error: List comprehension has incompatible type List[UploadedFile]; expected List[Tuple[Any, ...]]
filemanager/process/strategy.py:75: error: Call to untyped function "await_completion" in typed context
filemanager/process/strategy.py:84: error: "IChecker" has no attribute "__iter__" (not iterable)
filemanager/process/strategy.py:95: error: No return value expected
filemanager/process/check/file_type.py:399: error: Argument 1 to "_type_of_latex2e" has incompatible type "UploadedFile"; expected "BytesIO"
filemanager/process/check/file_type.py:435: error: Incompatible types in assignment (expression has type "bytes", variable has type "str")
filemanager/process/check/file_type.py:464: error: Argument 1 to "_type_of_latex2e" has incompatible type "IO[Any]"; expected "BytesIO"
filemanager/process/check/cleanup.py:171: error: Argument 1 to "findall" of "Pattern" has incompatible type "mmap"; expected "bytes"
filemanager/process/check/cleanup.py:226: error: Argument 1 to "search" of "Pattern" has incompatible type "str"; expected "bytes"
filemanager/process/check/cleanup.py:233: error: Argument 1 to "search" of "Pattern" has incompatible type "str"; expected "bytes"
filemanager/process/check/cleanup.py:239: error: Argument 1 to "search" of "Pattern" has incompatible type "str"; expected "bytes"
filemanager/process/check/cleanup.py:247: error: Argument 1 to "search" of "Pattern" has incompatible type "str"; expected "bytes"
filemanager/process/check/cleanup.py:253: error: Unsupported operand types for + ("bytes" and "str")
filemanager/process/check/cleanup.py:256: error: Argument 1 to "search" of "Pattern" has incompatible type "str"; expected "bytes"
filemanager/controllers/checkpoint.py:122: error: "print_exc" does not return a value
filemanager/controllers/checkpoint.py:233: error: "print_exc" does not return a value
filemanager/controllers/checkpoint.py:282: error: "print_exc" does not return a value
filemanager/controllers/upload.py:101: error: Incompatible types in assignment (expression has type "None", variable has type "UploadWorkspace")
filemanager/controllers/upload.py:177: error: Argument 1 to "create" of "FileMutationsWorkspace" has incompatible type "Union[str, None, Any]"; expected "str"
filemanager/controllers/files.py:221: error: "UploadFileSecurityError" has no attribute "description"

DavidLFielding and others added 29 commits May 1, 2019 20:51

Added low level checkpoint routines to create/list/remove[all], and r…

9b287d1

…estore checkpoints.

Tests for low level checkpoint routines.

cb2d72f

Add support for checkpoint download.

9a3b414

Add support for checkpoint download.

4ea54af

Added tests for checkpoint API requests. [ARXIVNG-2032] David

fa0038e

Add tests for checkpopint download support routines. [ARXIVNG-2032] D…

c684c94

…avid

Added routes to support checkpoint creation/listing/deletion/download…

07ca13f

…ing. [ARXIVNG-2032] David

Minor method name change.

2626d34

Adjust checkpoint file names.

c58aba3

Wire up checkpoint on upload. [ARXIVNG-2032] David

a9c9120

Add route which supports checkpoint on upload. [ARXIVNG-2032] David

130ffc1

Added tests for upload with checkpoint. [ARXIVNG-2032] David

67b7880

Add definitions for checkpoint requests. [ARXIVNG-2032] David

7e14fde

Update auth to get checkpoint scopes.

5a971a5

Update to use new checkpoint scopes from auth.

d980f5b

Update tests to use checkpoint scopes and some other minor cleanup of…

3264dde

… checkpoint tests.

Fixed type in test archive filename.

66b7f55

Add some temporary debugging for Travis

4d5b2c9

Add some temporary debugging for Travis

a73565c

Add some temporary debugging for Travis

1d7e878

Fixed typo

935d782

Fix typo

861d90a

Removed temporary debugging.

afef441

Docstring cleanup. Standardize logging user information.

fb376a8

Pass user information so we are able to log who is doing what to work…

6615e5f

…space.

Improve method names.

39ad524

General improvements to exception handling.

5939302

Mainly plint cleanup and some additional tests.

1d8262a

ARXIVNG-2210 initial support for API clients

13970d0

erickpeirson requested a review from DavidLFielding June 5, 2019 20:25

DavidLFielding reviewed Aug 11, 2019

View reviewed changes