Skip to content

[core][sandbox] Bound the image cache via LRU eviction v2 - #65964

Closed
pcmoritz wants to merge 4 commits into
ray-project:masterfrom
pcmoritz:sandbox-image-cache-runsc
Closed

[core][sandbox] Bound the image cache via LRU eviction v2#65964
pcmoritz wants to merge 4 commits into
ray-project:masterfrom
pcmoritz:sandbox-image-cache-runsc

Conversation

@pcmoritz

@pcmoritz pcmoritz commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

This is an alternative proposal to achieve #65748

It tries to improve on the original PR in a number of ways:

  1. Avoid re-computing the image size over and over, only do it once when the image is extracted
  2. Make it easier to follow the logic of which images are in use, by using the ground truth runsc command
  3. Allow eviction of unused images while other images are pulling or starting. Each pull/startup holds a shared <image>.startup.lock; eviction serializes on .cache.lock, acquires exclusive nonblocking candidate startup locks, then reads gVisor state once while retaining those locks through deletion.

Related issues

Related to #65748; this alternative uses gVisor state instead of persistent usage markers and saves image sizes at extraction.

Additional information

Image sizes and recency are refreshed from saved metadata after candidate locks are acquired. The extraction marker is written only after the image directory reaches its final path, preventing concurrent eviction of temporary extraction directories.

Validation: 61 selected unit tests passed; 8 tests were deselected. Tests ran in a virtual environment through a temporary harness that bypasses native Ray initialization and the Linux sandbox setup. The local native extension is out of sync with the checkout, so full gVisor integration was not run. The exclusions cover runsc/registry tests and an existing directory-permission failure also reproduced on unchanged code.

/private/tmp/ray-image-cache.f39odz/venv/bin/python /private/tmp/ray-image-cache.f39odz/run_unit_tests.py -q python/ray/experimental/sandbox/tests/test_image_cache.py python/ray/experimental/sandbox/tests/test_image_manager.py python/ray/experimental/sandbox/tests/test_image_utils.py -k 'not test_get_default_oci_spec and not test_image_manager_create_oci_spec and not test_image_manager_prepare_oci_bundle and not test_sandbox_runtime_image_manager_integration and not test_pull_and_extract_remote_image and not test_pull_and_extract_docker_io_prefixed_image and not test_pull_nonexistent_image and not test_extract_tar_layer_usr_merge'
/private/tmp/ray-image-cache.f39odz/venv/bin/python -m pre_commit run

All applicable pre-commit hooks passed. AI assistance was used for implementation and validation.

Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
@pcmoritz
pcmoritz requested review from a team as code owners September 6, 2026 02:20

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements a mechanism to bound the sandbox image cache by evicting least recently used images when the cache size exceeds a specified limit (defaulting to half of the filesystem). It integrates with runsc list to protect images currently in use and removes the creation of redundant uncompressed tar archives. The review feedback suggests checking for the existence of the runsc executable in the system PATH before running it to prevent noisy FileNotFoundError warnings in environments where gVisor is not installed.

Comment on lines +399 to +405
result = subprocess.run(
["runsc", "--root", RUNSC_ROOT, "list", "--format=json"],
capture_output=True,
text=True,
check=True,
timeout=10,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If runsc is not installed on the system (which is common in non-gVisor environments or during local development/testing), calling subprocess.run will raise a FileNotFoundError. While this is caught by the caller, it logs a noisy warning with a full stack trace. Checking if runsc is available in the system PATH using shutil.which before running the subprocess can prevent this log pollution.

    if not shutil.which("runsc"):
        logger.warning("gVisor executable 'runsc' not found in PATH; skipping image eviction.")
        return

    result = subprocess.run(
        ["runsc", "--root", RUNSC_ROOT, "list", "--format=json"],
        capture_output=True,
        text=True,
        check=True,
        timeout=10,
    )

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 8404ef8. Configure here.

os.utime(marker_path, None)
return target_dir
else:
os.utime(marker_path, None)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Old cache markers skip eviction

Medium Severity

Cache hits refresh the .extracted mtime but leave a pre-existing ok marker unchanged. Eviction then skips those directories as invalid sizes, so they never count toward the limit and are never reclaimed. On upgrade, the LRU bound does not apply to images already on disk.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8404ef8. Configure here.

Signed-off-by: Philipp Moritz <pcmoritz@gmail.com>
@pcmoritz pcmoritz closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant