Implement read-only local cache #2048

uellenberg · 2024-01-20T09:10:14Z

This allows the local cache to be set as read-only (similar to GCS read-only) when the SCCACHE_LOCAL_RW_MODE environment variable is set to READ_ONLY (or the corresponding config option).

Closes #1852.

uellenberg · 2024-01-20T09:15:46Z

src/compiler/c.rs

-                        storage.put_preprocessor_cache_entry(
+
+                        if let Err(e) = storage.put_preprocessor_cache_entry(
                            preprocessor_key,
                            preprocessor_cache_entry,
-                        )?;
+                        ) {
+                            debug!("Failed to update preprocessor cache: {}", e);
+                            update_failed = true;
+                        }


I'm unsure about this behavior. It's based on how compiler.rs handles errors.

Realistically, errors shouldn't be used here, but it seems to be how it's handled with GCS read-only, so I did it the same way. I thought about changing it, but I'd like to hear the input about someone more knowledgeable about sccache's internals first. Storage::check() seems to be a great candidate, although it only seems to be used for a print and is very expensive for GCS (which could be changed, of course).

uellenberg · 2024-01-20T09:15:59Z

src/compiler/c.rs

-                storage
-                    .put_preprocessor_cache_entry(&preprocessor_key, preprocessor_cache_entry)?;
+
+                if let Err(e) = storage
+                    .put_preprocessor_cache_entry(&preprocessor_key, preprocessor_cache_entry)
+                {
+                    debug!("Failed to update preprocessor cache: {}", e);
+                }


Same as above.

uellenberg · 2024-01-20T09:16:58Z

src/cache/gcs.rs

+impl From<config::CacheRWMode> for RWMode {
+    fn from(value: config::CacheRWMode) -> Self {
+        match value {
+            config::CacheRWMode::ReadOnly => RWMode::ReadOnly,
+            config::CacheRWMode::ReadWrite => RWMode::ReadWrite,
+        }
+    }
+}
+


There are a few different types like this that have the same purpose. Should they be consolidated?

I've merged two of them together, although there's still CacheModeConfig and CacheMode, which do effectively the same thing.

codecov-commenter · 2024-01-20T10:24:14Z

Codecov Report

Attention: 143 lines in your changes are missing coverage. Please review.

Comparison is base (8d3cd35) 30.86% compared to head (c765ec1) 30.85%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
tests/sccache_cargo.rs	21.59%	9 Missing and 60 partials ⚠️
src/compiler/c.rs	14.28%	11 Missing and 13 partials ⚠️
src/cache/cache.rs	34.61%	2 Missing and 15 partials ⚠️
src/config.rs	40.00%	13 Missing and 2 partials ⚠️
src/cache/disk.rs	11.11%	1 Missing and 7 partials ⚠️
src/compiler/compiler.rs	0.00%	0 Missing and 5 partials ⚠️
src/cache/gcs.rs	33.33%	2 Missing and 2 partials ⚠️
src/test/tests.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2048      +/-   ##
==========================================
- Coverage   30.86%   30.85%   -0.02%     
==========================================
  Files          51       52       +1     
  Lines       19653    19889     +236     
  Branches     9455     9620     +165     
==========================================
+ Hits         6066     6136      +70     
- Misses       7852     7911      +59     
- Partials     5735     5842     +107

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sylvestre · 2024-01-22T09:59:13Z

docs/Local.md

+
+## Read-only cache mode
+
+By default, the local cache operates in read/write mode. The `SCCACHE_LOCAL_RW_MODE` environment variable can be set to `READ_ONLY` (or `READ_WRITE`) to modify this behavior.


could you please detail in which case someone might want to use READ_ONLY
thanks

could you please detail in which case someone might want to use READ_ONLY thanks

@sylvestre

Done, let me know if that's what you want. For a more specific use case, see the linked issue. I'm using sccache to speed up build times for untrusted (and short-lived) build tasks, where new items can't be added as they may not be valid, but caching commonly used libraries can still speed up build times immensely. Read-only is not being used as a security feature, as build tasks are already isolated correctly, but to reduce useless disk consumption.

docs/Local.md

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>

sylvestre · 2024-01-25T13:00:06Z

src/cache/disk.rs

@@ -130,6 +133,11 @@ impl Storage for DiskCache {
        // We should probably do this on a background thread if we're going to buffer
        // everything in memory...
        trace!("DiskCache::finish_put({})", key);
+
+        if self.rw_mode == CacheMode::ReadOnly {
+            return Err(anyhow!("Cannot write to a read-only cache"));


could you please add a test to trigger this error ? thanks

could you please add a test to trigger this error ? thanks

@sylvestre Done.

could you please check for the error message string too ?
thanks

uellenberg · 2024-01-26T08:39:58Z

I'm not sure why the integration tests are failing. The only change I made (since they succeeded) was asserting the errors' messages and running rustfmt.

sylvestre · 2024-01-26T08:50:45Z

rate limiting on github, not your fault :)

sylvestre · 2024-01-26T08:51:08Z

thanks for your pr!

Implement read-only local cache

081c4f2

uellenberg commented Jan 20, 2024

View reviewed changes

uellenberg added 5 commits January 20, 2024 02:58

Fix build errors with dist-server

e910ff4

Add test for local read-only cache

843448a

Fix local read-only mode error messages

beac2ec

Merge RWMode into CacheMode

1bf9f61

Rename CacheRWMode to CacheModeConfig

609b405

sylvestre reviewed Jan 22, 2024

View reviewed changes

Expand local read-only documentation

8c54c2f

sylvestre reviewed Jan 22, 2024

View reviewed changes

docs/Local.md Outdated Show resolved Hide resolved

Clarify how local read-only mode can be misused

805e4ef

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>

sylvestre reviewed Jan 25, 2024

View reviewed changes

uellenberg added 3 commits January 25, 2024 10:29

Test that the disk cache will not accept writes in read-only mode

b065f20

Fix Clippy

c765ec1

Check error message for read-only mode test

6364e35

sylvestre merged commit 742d126 into mozilla:main Jan 26, 2024
45 of 48 checks passed

Porkepix mentioned this pull request Feb 7, 2024

sccache 0.7.7 Homebrew/homebrew-core#162078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement read-only local cache #2048

Implement read-only local cache #2048

uellenberg commented Jan 20, 2024

uellenberg Jan 20, 2024

uellenberg Jan 20, 2024

uellenberg Jan 20, 2024

uellenberg Jan 21, 2024

codecov-commenter commented Jan 20, 2024 •

edited

Loading

sylvestre Jan 22, 2024

uellenberg Jan 22, 2024

sylvestre Jan 25, 2024

uellenberg Jan 25, 2024

sylvestre Jan 26, 2024

uellenberg commented Jan 26, 2024

sylvestre commented Jan 26, 2024

sylvestre commented Jan 26, 2024


		## Read-only cache mode

		By default, the local cache operates in read/write mode. The `SCCACHE_LOCAL_RW_MODE` environment variable can be set to `READ_ONLY` (or `READ_WRITE`) to modify this behavior.

Implement read-only local cache #2048

Implement read-only local cache #2048

Conversation

uellenberg commented Jan 20, 2024

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

codecov-commenter commented Jan 20, 2024 • edited Loading

Codecov Report

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

uellenberg commented Jan 26, 2024

sylvestre commented Jan 26, 2024

sylvestre commented Jan 26, 2024

codecov-commenter commented Jan 20, 2024 •

edited

Loading