Make `contents` API scale #3609

dbutenhof · 2024-02-16T15:31:58Z

PBENCH-1321

The /datasets/{id}/contents API includes into several unexpectedly expensive steps:

Finding the tarball (by MD5 value) within the ARCHIVE tree using a glob
Fully discovering all tarballs within the controller directory
Unpacking the tarball into a cache directory using tar
Building a "map" of the contents of the unpacked tarball subtree

This PR includes mitigations for all but the tar unpack step:

Use the server.tarball-path metadata instead of searching the disk
Only discover the target tarball rather than the entire controller
Skip the "map" and evaluate the actual target path within the cache

Finding a tarball within our 30Tb ARCHIVE tree can take many minutes, while identifying the controller directory from the tarball path takes a fraction of a second.

Depending on the number of tarballs within a controller (some have many), full controller discovery has been observed to take half a minute; while populating only the target tarball takes a fraction of a second.

Building the map for a large tarball tree can take minutes, whereas discovery of the actual relative file path within the cache runs at native (Python) file system speeds.

webbnh

Looks good; however, I have the usual collection of nits, pointed questions, and suggestions.

webbnh · 2024-02-19T19:40:02Z

lib/pbench/server/cache_manager.py

+                            if target.is_dir():
+                                uri = f"{origin}/contents/{link}"
+                                type = CacheType.DIRECTORY
+                                append_to = dir_list
+                            elif target.is_file():
+                                uri = f"{origin}/inventory/{link}"
+                                type = CacheType.FILE
+                            else:
+                                uri = f"{origin}/inventory/{relative}"
+                                type = CacheType.OTHER


I'm confused: why is the uri sometimes built from link and sometimes built from relative?

For OTHER and BROKEN the URL points to the symlink since we can't (or won't) resolve it. For others, we return the resolved target so they get the real directory or file.

I concur on the BROKEN case...but why is that appropriate for the OTHER case? (In the OTHER case, we successfully resolved the symlink to a directory entry accessible within the tarball via a relative path, so won't f"{origin}/inventory/{link}" and f"{origin}/inventory/{relative}" address the same file, with the latter just taking a more circuitous route, if the route is different at all?)

That's actually a comment for inventory rather than contents. We don't want to expose the real target of an OTHER file (e.g., a fifo or whatever), which is why we use OTHER. Yeah, it's possible that inventory will transparently resolve the link and return the fifo, which I suspect we really don't want. I'm not going to mess with that here.

lib/pbench/server/cache_manager.py

lib/pbench/test/unit/server/test_cache_manager.py

dbutenhof

I made some of the changes, including some new test links.

I also got an idea this afternoon (which is why I'm working way late today!) about improving the report generator performance by extending my find_dataset changes to an alternate "full discovery" by pulling all the dataset resource IDs and tarball-path values from SQL instead of iterdir-ing through the ARCHIVEs. Turns out that cuts cache discovery from nearly 2 hours to about 25 minutes.

lib/pbench/server/cache_manager.py

dbutenhof · 2024-02-20T00:39:37Z

lib/pbench/server/cache_manager.py

+                            if target.is_dir():
+                                uri = f"{origin}/contents/{link}"
+                                type = CacheType.DIRECTORY
+                                append_to = dir_list
+                            elif target.is_file():
+                                uri = f"{origin}/inventory/{link}"
+                                type = CacheType.FILE
+                            else:
+                                uri = f"{origin}/inventory/{relative}"
+                                type = CacheType.OTHER


For OTHER and BROKEN the URL points to the symlink since we can't (or won't) resolve it. For others, we return the resolved target so they get the real directory or file.

lib/pbench/server/cache_manager.py

lib/pbench/test/unit/server/test_cache_manager.py

webbnh

The updates look generally good. I've got follow-ups on two of my comments from the first pass (the rest are resolved -- thanks!), and I've got a few thoughts on your revisions. The foremost is that making CacheManager.full_discovery() into a wrapper seems like it causes more problems than it really solves. (The result is good, but the approach has some flaws.) The others are smaller bits, nits, and suggestions.

lib/pbench/cli/server/report.py

lib/pbench/server/cache_manager.py

webbnh · 2024-02-20T17:33:44Z

lib/pbench/server/cache_manager.py

+                            if target.is_dir():
+                                uri = f"{origin}/contents/{link}"
+                                type = CacheType.DIRECTORY
+                                append_to = dir_list
+                            elif target.is_file():
+                                uri = f"{origin}/inventory/{link}"
+                                type = CacheType.FILE
+                            else:
+                                uri = f"{origin}/inventory/{relative}"
+                                type = CacheType.OTHER


I concur on the BROKEN case...but why is that appropriate for the OTHER case? (In the OTHER case, we successfully resolved the symlink to a directory entry accessible within the tarball via a relative path, so won't f"{origin}/inventory/{link}" and f"{origin}/inventory/{relative}" address the same file, with the latter just taking a more circuitous route, if the route is different at all?)

lib/pbench/server/cache_manager.py

lib/pbench/test/unit/server/test_cache_manager.py

PBENCH-1321 The `/datasets/{id}/contents` API includes into several unexpectedly expensive steps: 1. Finding the tarball (by MD5 value) within the `ARCHIVE` tree using a `glob` 2. Fully discovering all tarballs within the controller directory 3. Unpacking the tarball into a cache directory using `tar` 4. Building a "map" of the contents of the unpacked tarball subtree This PR includes mitigations for all but the `tar` unpack step: 1. Use the `server.tarball-path` metadata instead of searching the disk 2. Only discover the target tarball rather than the entire controller 3. Skip the "map" and evaluate the actual target path within the cache Finding a tarball within our 30Tb `ARCHIVE` tree can take many minutes, while identifying the controller directory from the tarball path takes a fraction of a second. Depending on the number of tarballs within a controller (some have many), full controller discovery has been observed to take half a minute; while populating only the target tarball takes a fraction of a second. Building the map for a large tarball tree can take minutes, whereas discovery of the actual relative file path within the cache runs at native (Python) file system speeds.

webbnh

Looks good.

Are you contemplating acting on or responding to the two remaining open conversations (1, 2) or should I just close them?

dbutenhof

Apparently I have several responses written earlier this afternoon that didn't get posted. Huh. You can enjoy them at your leisure.

lib/pbench/server/cache_manager.py

lib/pbench/cli/server/report.py

dbutenhof · 2024-02-20T22:48:33Z

lib/pbench/server/cache_manager.py

+                            if target.is_dir():
+                                uri = f"{origin}/contents/{link}"
+                                type = CacheType.DIRECTORY
+                                append_to = dir_list
+                            elif target.is_file():
+                                uri = f"{origin}/inventory/{link}"
+                                type = CacheType.FILE
+                            else:
+                                uri = f"{origin}/inventory/{relative}"
+                                type = CacheType.OTHER


That's actually a comment for inventory rather than contents. We don't want to expose the real target of an OTHER file (e.g., a fifo or whatever), which is why we use OTHER. Yeah, it's possible that inventory will transparently resolve the link and return the fifo, which I suspect we really don't want. I'm not going to mess with that here.

lib/pbench/server/cache_manager.py

dbutenhof added bug Server Code Infrastructure Dashboard Of and relating to the Dashboard GUI API Of and relating to application programming interfaces to services and functions Operations Related to operation and monitoring of a service labels Feb 16, 2024

dbutenhof requested a review from webbnh February 16, 2024 15:31

dbutenhof self-assigned this Feb 16, 2024

dbutenhof marked this pull request as draft February 16, 2024 16:32

This comment was marked as outdated.

Sign in to view

dbutenhof marked this pull request as ready for review February 16, 2024 23:16

webbnh previously approved these changes Feb 19, 2024

View reviewed changes

dbutenhof commented Feb 20, 2024

View reviewed changes

dbutenhof dismissed webbnh’s stale review via f7f113f February 20, 2024 13:26

webbnh previously approved these changes Feb 20, 2024

View reviewed changes

dbutenhof added 5 commits February 20, 2024 17:18

Take 2: clean up, more tests

a44a87d

More tests, remove some logging

f738338

Add test cases, optimize full discovery

aa01ac0

Tweaks and nits

9ccd500

dbutenhof dismissed webbnh’s stale review via 9ccd500 February 20, 2024 22:23

dbutenhof force-pushed the content branch from f7f113f to 9ccd500 Compare February 20, 2024 22:23

webbnh approved these changes Feb 20, 2024

View reviewed changes

dbutenhof commented Feb 20, 2024

View reviewed changes

dbutenhof merged commit c0946eb into distributed-system-analysis:main Feb 21, 2024
4 checks passed

dbutenhof deleted the content branch February 21, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `contents` API scale #3609

Make `contents` API scale #3609

dbutenhof commented Feb 16, 2024

This comment was marked as outdated.

webbnh left a comment

webbnh Feb 19, 2024

dbutenhof Feb 20, 2024

webbnh Feb 20, 2024 •

edited

Loading

dbutenhof Feb 20, 2024

dbutenhof left a comment

dbutenhof Feb 20, 2024

webbnh left a comment

webbnh Feb 20, 2024 •

edited

Loading

webbnh left a comment

dbutenhof left a comment

dbutenhof Feb 20, 2024

Make contents API scale #3609

Make contents API scale #3609

Conversation

dbutenhof commented Feb 16, 2024

This comment was marked as outdated.

webbnh left a comment

Choose a reason for hiding this comment

webbnh Feb 19, 2024

Choose a reason for hiding this comment

dbutenhof Feb 20, 2024

Choose a reason for hiding this comment

webbnh Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

dbutenhof Feb 20, 2024

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof Feb 20, 2024

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof Feb 20, 2024

Choose a reason for hiding this comment

Make `contents` API scale #3609

Make `contents` API scale #3609

webbnh Feb 20, 2024 •

edited

Loading

webbnh Feb 20, 2024 •

edited

Loading