From 73f87ca17ef1584abacc4b76ff2641b48c847a86 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 22 Mar 2021 16:41:54 -0700 Subject: [PATCH 1/4] Allow setting the lease time and store sizes from Python. --- .../pants/engine/internals/native_engine.pyi | 5 +- .../pants/engine/internals/scheduler.py | 11 +- src/rust/engine/fs/brfs/src/main.rs | 29 +++-- src/rust/engine/fs/fs_util/src/main.rs | 20 ++- src/rust/engine/fs/store/src/lib.rs | 52 +++++--- src/rust/engine/fs/store/src/local.rs | 34 ++--- src/rust/engine/fs/store/src/remote.rs | 2 + src/rust/engine/fs/store/src/tests.rs | 120 +++++++++--------- .../src/remote_cache_tests.rs | 24 ++-- .../process_execution/src/remote_tests.rs | 120 +++++++++--------- src/rust/engine/process_executor/src/main.rs | 10 +- src/rust/engine/src/context.rs | 58 ++++++--- src/rust/engine/src/externs/interface.rs | 36 +++++- src/rust/engine/src/lib.rs | 2 +- 14 files changed, 295 insertions(+), 228 deletions(-) diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index 555243f64eb..b6fa6676183 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -99,13 +99,13 @@ def scheduler_create( tasks: PyTasks, types: PyTypes, build_root: str, - local_store_dir: str, local_execution_root_dir: str, named_caches_dir: str, ca_certs_path: str | None, ignore_patterns: Sequence[str], use_gitignore: bool, remoting_options: PyRemotingOptions, + local_store_options: PyLocalStoreOptions, exec_strategy_opts: PyExecutionStrategyOptions, ) -> PyScheduler: ... def scheduler_execute( @@ -183,6 +183,9 @@ class PyNailgunClient: class PyRemotingOptions: def __init__(self, **kwargs: Any) -> None: ... +class PyLocalStoreOptions: + def __init__(self, **kwargs: Any) -> None: ... + class PyScheduler: pass diff --git a/src/python/pants/engine/internals/scheduler.py b/src/python/pants/engine/internals/scheduler.py index 68c2e588f2a..94c15a72473 100644 --- a/src/python/pants/engine/internals/scheduler.py +++ b/src/python/pants/engine/internals/scheduler.py @@ -37,6 +37,7 @@ PyExecutionRequest, PyExecutionStrategyOptions, PyExecutor, + PyLocalStoreOptions, PyRemotingOptions, PyScheduler, PySession, @@ -176,6 +177,14 @@ def __init__( execution_headers=tuple(execution_options.remote_execution_headers.items()), execution_overall_deadline_secs=execution_options.remote_execution_overall_deadline_secs, ) + # TODO: Expose options. + local_store_options = PyLocalStoreOptions( + store_dir=local_store_dir, + process_cache_max_size_bytes=(2 * 4 * 1000000000), + files_max_size_bytes=(16 * 4 * 1000000000), + directories_max_size_bytes=(2 * 4 * 1000000000), + lease_time_millis=(2 * 60 * 60 * 1000), + ) exec_stategy_opts = PyExecutionStrategyOptions( local_parallelism=execution_options.process_execution_local_parallelism, remote_parallelism=execution_options.process_execution_remote_parallelism, @@ -190,13 +199,13 @@ def __init__( tasks, types, build_root, - local_store_dir, local_execution_root_dir, named_caches_dir, ca_certs_path, ignore_patterns, use_gitignore, remoting_options, + local_store_options, exec_stategy_opts, ) diff --git a/src/rust/engine/fs/brfs/src/main.rs b/src/rust/engine/fs/brfs/src/main.rs index 3885304a29f..868f6160c9d 100644 --- a/src/rust/engine/fs/brfs/src/main.rs +++ b/src/rust/engine/fs/brfs/src/main.rs @@ -734,21 +734,22 @@ async fn main() { let runtime = task_executor::Executor::new(); + let local_only_store = + Store::local_only(runtime.clone(), &store_path).expect("Error making local store."); let store = match args.value_of("server-address") { - Some(address) => Store::with_remote( - runtime.clone(), - &store_path, - address, - args.value_of("remote-instance-name").map(str::to_owned), - root_ca_certs, - headers, - 4 * 1024 * 1024, - std::time::Duration::from_secs(5 * 60), - 1, - ), - None => Store::local_only(runtime.clone(), &store_path), - } - .expect("Error making store"); + Some(address) => local_only_store + .into_with_remote( + address, + args.value_of("remote-instance-name").map(str::to_owned), + root_ca_certs, + headers, + 4 * 1024 * 1024, + std::time::Duration::from_secs(5 * 60), + 1, + ) + .expect("Error making remote store"), + None => local_only_store, + }; #[derive(Clone, Copy, Debug)] enum Sig { diff --git a/src/rust/engine/fs/fs_util/src/main.rs b/src/rust/engine/fs/fs_util/src/main.rs index f9082fb5c68..1c3e853d5a0 100644 --- a/src/rust/engine/fs/fs_util/src/main.rs +++ b/src/rust/engine/fs/fs_util/src/main.rs @@ -263,6 +263,12 @@ async fn execute(top_match: &clap::ArgMatches<'_>) -> Result<(), ExitError> { .unwrap_or_else(Store::default_path); let runtime = task_executor::Executor::new(); let (store, store_has_remote) = { + let local_only = Store::local_only(runtime.clone(), &store_dir).map_err(|e| { + format!( + "Failed to open/create store for directory {:?}: {}", + store_dir, e + ) + })?; let (store_result, store_has_remote) = match top_match.value_of("server-address") { Some(cas_address) => { let chunk_size = @@ -292,9 +298,7 @@ async fn execute(top_match: &clap::ArgMatches<'_>) -> Result<(), ExitError> { } ( - Store::with_remote( - runtime.clone(), - &store_dir, + local_only.into_with_remote( cas_address, top_match .value_of("remote-instance-name") @@ -316,15 +320,9 @@ async fn execute(top_match: &clap::ArgMatches<'_>) -> Result<(), ExitError> { true, ) } - None => (Store::local_only(runtime.clone(), &store_dir), false), + None => (Ok(local_only), false), }; - let store = store_result.map_err(|e| { - format!( - "Failed to open/create store for directory {:?}: {}", - store_dir, e - ) - })?; - (store, store_has_remote) + (store_result?, store_has_remote) }; match top_match.subcommand() { diff --git a/src/rust/engine/fs/store/src/lib.rs b/src/rust/engine/fs/store/src/lib.rs index 11111d0a2e2..04f4791e64c 100644 --- a/src/rust/engine/fs/store/src/lib.rs +++ b/src/rust/engine/fs/store/src/lib.rs @@ -48,6 +48,7 @@ use futures::future::{self, BoxFuture, Either, FutureExt, TryFutureExt}; use grpc_util::prost::MessageExt; use hashing::Digest; use serde_derive::Serialize; +use sharded_lmdb::DEFAULT_LEASE_TIME; use tryfuture::try_future; use std::collections::{BTreeMap, HashMap, HashSet}; @@ -65,15 +66,6 @@ use remexec::Tree; const MEGABYTES: usize = 1024 * 1024; const GIGABYTES: usize = 1024 * MEGABYTES; -/// -/// This is the target number of bytes which should be present in all combined LMDB store files -/// after garbage collection. We almost certainly want to make this configurable. -/// -/// Because LMDB is sharded (by ShardedLmdb::NUM_SHARDS), this value also bounds the maximum size -/// of individual items in the store: see the relevant calls to `ShardedLmdb::new` for more info. -/// -pub const DEFAULT_LOCAL_STORE_GC_TARGET_BYTES: usize = 4 * GIGABYTES; - mod local; #[cfg(test)] pub mod local_tests; @@ -82,6 +74,26 @@ mod remote; #[cfg(test)] mod remote_tests; +pub struct LocalOptions { + pub files_max_size_bytes: usize, + pub directories_max_size_bytes: usize, + pub lease_time: Duration, +} + +/// +/// NB: These defaults are intended primarily for use in tests: high level code should expose +/// explicit settings in most cases. +/// +impl Default for LocalOptions { + fn default() -> Self { + Self { + files_max_size_bytes: 16 * 4 * GIGABYTES, + directories_max_size_bytes: 2 * 4 * GIGABYTES, + lease_time: DEFAULT_LEASE_TIME, + } + } +} + // Summary of the files and directories uploaded with an operation // ingested_file_{count, bytes}: Number and combined size of processed files // uploaded_file_{count, bytes}: Number and combined size of files uploaded to the remote @@ -237,6 +249,17 @@ impl Store { }) } + pub fn local_only_with_options>( + executor: task_executor::Executor, + path: P, + options: LocalOptions, + ) -> Result { + Ok(Store { + local: local::ByteStore::new_with_options(executor, path, options)?, + remote: None, + }) + } + /// /// Converts this (copy of) a Store to local only by dropping the remote half. /// @@ -251,12 +274,11 @@ impl Store { } /// - /// Make a store which uses local storage, and if it is missing a value which it tries to load, - /// will attempt to back-fill its local storage from a remote CAS. + /// Add remote storage to a Store. If it is missing a value which it tries to load, it will + /// attempt to back-fill its local storage from the remote storage. /// - pub fn with_remote>( - executor: task_executor::Executor, - path: P, + pub fn into_with_remote( + self, cas_address: &str, instance_name: Option, root_ca_certs: Option>, @@ -266,7 +288,7 @@ impl Store { rpc_retries: usize, ) -> Result { Ok(Store { - local: local::ByteStore::new(executor, path)?, + local: self.local, remote: Some(remote::ByteStore::new( cas_address, instance_name, diff --git a/src/rust/engine/fs/store/src/local.rs b/src/rust/engine/fs/store/src/local.rs index bb7fa7879f1..05aa7648f2e 100644 --- a/src/rust/engine/fs/store/src/local.rs +++ b/src/rust/engine/fs/store/src/local.rs @@ -1,4 +1,4 @@ -use super::{EntryType, ShrinkBehavior, DEFAULT_LOCAL_STORE_GC_TARGET_BYTES}; +use super::{EntryType, ShrinkBehavior}; use std::collections::BinaryHeap; use std::path::Path; @@ -10,7 +10,7 @@ use futures::future; use hashing::{Digest, Fingerprint, EMPTY_DIGEST}; use lmdb::Error::NotFound; use lmdb::{self, Cursor, Transaction}; -use sharded_lmdb::{ShardedLmdb, VersionedFingerprint, DEFAULT_LEASE_TIME}; +use sharded_lmdb::{ShardedLmdb, VersionedFingerprint}; use workunit_store::ObservationMetric; #[derive(Debug, Clone)] @@ -33,47 +33,31 @@ impl ByteStore { executor: task_executor::Executor, path: P, ) -> Result { - Self::new_with_lease_time(executor, path, DEFAULT_LEASE_TIME) + Self::new_with_options(executor, path, super::LocalOptions::default()) } - pub fn new_with_lease_time>( + pub fn new_with_options>( executor: task_executor::Executor, path: P, - lease_time: Duration, + options: super::LocalOptions, ) -> Result { let root = path.as_ref(); let files_root = root.join("files"); let directories_root = root.join("directories"); Ok(ByteStore { inner: Arc::new(InnerStore { - // The size value bounds the total size of all shards, but it also bounds the per item - // size to `max_size / ShardedLmdb::NUM_SHARDS`. - // - // We want these stores to be allowed to grow to a large multiple of the `StoreGCService` - // size target (see DEFAULT_LOCAL_STORE_GC_TARGET_BYTES), in case it is a very long time - // between GC runs. It doesn't reflect space allocated on disk, or RAM allocated (it may - // be reflected in VIRT but not RSS). - // - // However! We set them lower than we'd otherwise choose for a few reasons: - // 1. we see tests on travis fail because they can't allocate virtual memory if there - // are too many open Stores at the same time. - // 2. macOS creates core dumps that apparently include MMAP'd pages, and setting this too - // high will use up an unnecessary amount of disk if core dumps are enabled. - // file_dbs: ShardedLmdb::new( files_root, - // We set this larger than we do other stores in order to avoid applying too low a bound - // on the size of stored files. - 16 * DEFAULT_LOCAL_STORE_GC_TARGET_BYTES, + options.files_max_size_bytes, executor.clone(), - lease_time, + options.lease_time, ) .map(Arc::new), directory_dbs: ShardedLmdb::new( directories_root, - 2 * DEFAULT_LOCAL_STORE_GC_TARGET_BYTES, + options.directories_max_size_bytes, executor.clone(), - lease_time, + options.lease_time, ) .map(Arc::new), executor, diff --git a/src/rust/engine/fs/store/src/remote.rs b/src/rust/engine/fs/store/src/remote.rs index 8b0af745bb4..9d00ae954c8 100644 --- a/src/rust/engine/fs/store/src/remote.rs +++ b/src/rust/engine/fs/store/src/remote.rs @@ -39,6 +39,8 @@ impl fmt::Debug for ByteStore { } impl ByteStore { + // TODO: Consider extracting these options to a struct with `impl Default`, similar to + // `super::LocalOptions`. pub fn new( cas_address: &str, instance_name: Option, diff --git a/src/rust/engine/fs/store/src/tests.rs b/src/rust/engine/fs/store/src/tests.rs index 8628c8710fd..e6fe7b6727f 100644 --- a/src/rust/engine/fs/store/src/tests.rs +++ b/src/rust/engine/fs/store/src/tests.rs @@ -97,18 +97,18 @@ fn new_local_store>(dir: P) -> Store { /// Create a new store with a remote CAS. /// fn new_store>(dir: P, cas_address: &str) -> Store { - Store::with_remote( - task_executor::Executor::new(), - dir, - cas_address, - None, - None, - BTreeMap::new(), - 10 * MEGABYTES, - Duration::from_secs(1), - 1, - ) - .unwrap() + Store::local_only(task_executor::Executor::new(), dir) + .unwrap() + .into_with_remote( + cas_address, + None, + None, + BTreeMap::new(), + 10 * MEGABYTES, + Duration::from_secs(1), + 1, + ) + .unwrap() } #[tokio::test] @@ -837,18 +837,18 @@ async fn instance_name_upload() { .await .expect("Error storing catnip locally"); - let store_with_remote = Store::with_remote( - task_executor::Executor::new(), - dir.path(), - &cas.address(), - Some("dark-tower".to_owned()), - None, - BTreeMap::new(), - 10 * MEGABYTES, - Duration::from_secs(1), - 1, - ) - .unwrap(); + let store_with_remote = Store::local_only(task_executor::Executor::new(), dir.path()) + .unwrap() + .into_with_remote( + &cas.address(), + Some("dark-tower".to_owned()), + None, + BTreeMap::new(), + 10 * MEGABYTES, + Duration::from_secs(1), + 1, + ) + .unwrap(); store_with_remote .ensure_remote_has_recursive(vec![testdir.digest()]) @@ -864,18 +864,18 @@ async fn instance_name_download() { .file(&TestData::roland()) .build(); - let store_with_remote = Store::with_remote( - task_executor::Executor::new(), - dir.path(), - &cas.address(), - Some("dark-tower".to_owned()), - None, - BTreeMap::new(), - 10 * MEGABYTES, - Duration::from_secs(1), - 1, - ) - .unwrap(); + let store_with_remote = Store::local_only(task_executor::Executor::new(), dir.path()) + .unwrap() + .into_with_remote( + &cas.address(), + Some("dark-tower".to_owned()), + None, + BTreeMap::new(), + 10 * MEGABYTES, + Duration::from_secs(1), + 1, + ) + .unwrap(); assert_eq!( store_with_remote @@ -913,18 +913,18 @@ async fn auth_upload() { let mut headers = BTreeMap::new(); headers.insert("authorization".to_owned(), "Bearer Armory.Key".to_owned()); - let store_with_remote = Store::with_remote( - task_executor::Executor::new(), - dir.path(), - &cas.address(), - None, - None, - headers, - 10 * MEGABYTES, - Duration::from_secs(1), - 1, - ) - .unwrap(); + let store_with_remote = Store::local_only(task_executor::Executor::new(), dir.path()) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + headers, + 10 * MEGABYTES, + Duration::from_secs(1), + 1, + ) + .unwrap(); store_with_remote .ensure_remote_has_recursive(vec![testdir.digest()]) @@ -942,18 +942,18 @@ async fn auth_download() { let mut headers = BTreeMap::new(); headers.insert("authorization".to_owned(), "Bearer Armory.Key".to_owned()); - let store_with_remote = Store::with_remote( - task_executor::Executor::new(), - dir.path(), - &cas.address(), - None, - None, - headers, - 10 * MEGABYTES, - Duration::from_secs(1), - 1, - ) - .unwrap(); + let store_with_remote = Store::local_only(task_executor::Executor::new(), dir.path()) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + headers, + 10 * MEGABYTES, + Duration::from_secs(1), + 1, + ) + .unwrap(); assert_eq!( store_with_remote diff --git a/src/rust/engine/process_execution/src/remote_cache_tests.rs b/src/rust/engine/process_execution/src/remote_cache_tests.rs index 468975ac590..f323fa830bc 100644 --- a/src/rust/engine/process_execution/src/remote_cache_tests.rs +++ b/src/rust/engine/process_execution/src/remote_cache_tests.rs @@ -83,18 +83,18 @@ impl StoreSetup { let executor = task_executor::Executor::new(); let cas = StubCAS::builder().build(); let store_dir = TempDir::new().unwrap().path().join("store_dir"); - let store = Store::with_remote( - executor.clone(), - store_dir.clone(), - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .unwrap(); + let store = Store::local_only(executor.clone(), store_dir.clone()) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap(); StoreSetup { store, store_dir, diff --git a/src/rust/engine/process_execution/src/remote_tests.rs b/src/rust/engine/process_execution/src/remote_tests.rs index 52b24af6594..61abc74fa98 100644 --- a/src/rust/engine/process_execution/src/remote_tests.rs +++ b/src/rust/engine/process_execution/src/remote_tests.rs @@ -856,18 +856,18 @@ async fn sends_headers() { let cas = mock::StubCAS::empty(); let runtime = task_executor::Executor::new(); let store_dir = TempDir::new().unwrap(); - let store = Store::with_remote( - runtime.clone(), - store_dir, - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .expect("Failed to make store"); + let store = Store::local_only(runtime.clone(), store_dir) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap(); let command_runner = CommandRunner::new( &mock_server.address(), @@ -1051,18 +1051,18 @@ async fn ensure_inline_stdio_is_stored() { let store_dir_path = store_dir.path(); let cas = mock::StubCAS::empty(); - let store = Store::with_remote( - runtime.clone(), - &store_dir_path, - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .expect("Failed to make store"); + let store = Store::local_only(runtime.clone(), &store_dir_path) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap(); let cmd_runner = CommandRunner::new( &mock_server.address(), @@ -1428,18 +1428,18 @@ async fn execute_missing_file_uploads_if_known() { let cas = mock::StubCAS::builder() .directory(&TestDirectory::containing_roland()) .build(); - let store = Store::with_remote( - runtime.clone(), - store_dir, - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .expect("Failed to make store"); + let store = Store::local_only(runtime.clone(), store_dir) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap(); store .store_file_bytes(roland.bytes(), false) .await @@ -1503,18 +1503,18 @@ async fn execute_missing_file_errors_if_unknown() { .directory(&TestDirectory::containing_roland()) .build(); let runtime = task_executor::Executor::new(); - let store = Store::with_remote( - runtime.clone(), - store_dir, - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .expect("Failed to make store"); + let store = Store::local_only(runtime.clone(), store_dir) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap(); let runner = CommandRunner::new( &mock_server.address(), @@ -2264,18 +2264,18 @@ pub(crate) fn make_store( cas: &mock::StubCAS, executor: task_executor::Executor, ) -> Store { - Store::with_remote( - executor, - store_dir, - &cas.address(), - None, - None, - BTreeMap::new(), - 10 * 1024 * 1024, - Duration::from_secs(1), - 1, - ) - .expect("Failed to make store") + Store::local_only(executor, store_dir) + .unwrap() + .into_with_remote( + &cas.address(), + None, + None, + BTreeMap::new(), + 10 * 1024 * 1024, + Duration::from_secs(1), + 1, + ) + .unwrap() } async fn extract_execute_response( diff --git a/src/rust/engine/process_executor/src/main.rs b/src/rust/engine/process_executor/src/main.rs index 08370ebed6c..52ec8c81ac4 100644 --- a/src/rust/engine/process_executor/src/main.rs +++ b/src/rust/engine/process_executor/src/main.rs @@ -206,6 +206,8 @@ async fn main() { .clone() .unwrap_or_else(Store::default_path); + let local_only_store = + Store::local_only(executor.clone(), local_store_path).expect("Error making local store"); let store = match (&args.server, &args.cas_server) { (_, Some(cas_server)) => { let root_ca_certs = if let Some(ref path) = args.cas_root_ca_cert_file { @@ -224,9 +226,7 @@ async fn main() { ); } - Store::with_remote( - executor.clone(), - local_store_path, + local_only_store.into_with_remote( &cas_server, args.remote_instance_name.clone(), root_ca_certs, @@ -237,10 +237,10 @@ async fn main() { 3, ) } - (None, None) => Store::local_only(executor.clone(), local_store_path), + (None, None) => Ok(local_only_store), _ => panic!("Can't specify --server without --cas-server"), } - .expect("Error making store"); + .expect("Error making remote store"); let (mut request, process_metadata) = make_request(&store, &args) .await diff --git a/src/rust/engine/src/context.rs b/src/rust/engine/src/context.rs index 5e44537aaa0..b5fce39937c 100644 --- a/src/rust/engine/src/context.rs +++ b/src/rust/engine/src/context.rs @@ -26,8 +26,8 @@ use process_execution::{ }; use regex::Regex; use rule_graph::RuleGraph; -use sharded_lmdb::{ShardedLmdb, DEFAULT_LEASE_TIME}; -use store::{Store, DEFAULT_LOCAL_STORE_GC_TARGET_BYTES}; +use sharded_lmdb::ShardedLmdb; +use store::{self, Store}; use task_executor::Executor; use uuid::Uuid; use watch::{Invalidatable, InvalidationWatcher}; @@ -93,22 +93,44 @@ pub struct ExecutionStrategyOptions { pub remote_cache_write: bool, } +#[derive(Clone, Debug)] +pub struct LocalStoreOptions { + pub store_dir: PathBuf, + pub process_cache_max_size_bytes: usize, + pub files_max_size_bytes: usize, + pub directories_max_size_bytes: usize, + pub lease_time: Duration, +} + +impl From<&LocalStoreOptions> for store::LocalOptions { + fn from(lso: &LocalStoreOptions) -> Self { + Self { + files_max_size_bytes: lso.files_max_size_bytes, + directories_max_size_bytes: lso.directories_max_size_bytes, + lease_time: lso.lease_time, + } + } +} + impl Core { fn make_store( executor: &Executor, - local_store_dir: &Path, + local_store_options: &LocalStoreOptions, enable_remote: bool, remoting_opts: &RemotingOptions, remote_store_address: &Option, root_ca_certs: &Option>, ) -> Result { + let local_only = Store::local_only_with_options( + executor.clone(), + local_store_options.store_dir.clone(), + local_store_options.into(), + )?; if enable_remote { let remote_store_address = remote_store_address .as_ref() .ok_or("Remote store required, but none configured")?; - Store::with_remote( - executor.clone(), - local_store_dir, + local_only.into_with_remote( remote_store_address, remoting_opts.instance_name.clone(), root_ca_certs.clone(), @@ -118,7 +140,7 @@ impl Core { remoting_opts.store_rpc_retries, ) } else { - Store::local_only(executor.clone(), local_store_dir.to_path_buf()) + Ok(local_only) } } @@ -128,7 +150,7 @@ impl Core { executor: &Executor, local_execution_root_dir: &Path, named_caches_dir: &Path, - local_store_dir: &Path, + local_store_options: &LocalStoreOptions, process_execution_metadata: &ProcessMetadata, root_ca_certs: &Option>, exec_strategy_opts: &ExecutionStrategyOptions, @@ -198,10 +220,10 @@ impl Core { // Possibly use the local cache runner, regardless of remote execution/caching. let maybe_local_cached_command_runner = if exec_strategy_opts.use_local_cache { let process_execution_store = ShardedLmdb::new( - local_store_dir.join("processes"), - 2 * DEFAULT_LOCAL_STORE_GC_TARGET_BYTES, + local_store_options.store_dir.join("processes"), + local_store_options.process_cache_max_size_bytes, executor.clone(), - DEFAULT_LEASE_TIME, + local_store_options.lease_time, ) .map_err(|err| format!("Could not initialize store for process cache: {:?}", err))?; Box::new(process_execution::cache::CommandRunner::new( @@ -258,10 +280,10 @@ impl Core { build_root: PathBuf, ignore_patterns: Vec, use_gitignore: bool, - local_store_dir: PathBuf, local_execution_root_dir: PathBuf, named_caches_dir: PathBuf, ca_certs_path: Option, + local_store_options: LocalStoreOptions, remoting_opts: RemotingOptions, exec_strategy_opts: ExecutionStrategyOptions, ) -> Result { @@ -279,11 +301,15 @@ impl Core { || exec_strategy_opts.remote_cache_read || exec_strategy_opts.remote_cache_write; - safe_create_dir_all_ioerror(&local_store_dir) - .map_err(|e| format!("Error making directory {:?}: {:?}", local_store_dir, e))?; + safe_create_dir_all_ioerror(&local_store_options.store_dir).map_err(|e| { + format!( + "Error making directory {:?}: {:?}", + local_store_options.store_dir, e + ) + })?; let full_store = Self::make_store( &executor, - &local_store_dir, + &local_store_options, need_remote_store, &remoting_opts, &remoting_opts.store_address, @@ -315,7 +341,7 @@ impl Core { &executor, &local_execution_root_dir, &named_caches_dir, - &local_store_dir, + &local_store_options, &process_execution_metadata, &root_ca_certs, &exec_strategy_opts, diff --git a/src/rust/engine/src/externs/interface.rs b/src/rust/engine/src/externs/interface.rs index 05402f310b2..bee323be830 100644 --- a/src/rust/engine/src/externs/interface.rs +++ b/src/rust/engine/src/externs/interface.rs @@ -75,7 +75,8 @@ use workunit_store::{ use crate::{ externs, nodes, Core, ExecutionRequest, ExecutionStrategyOptions, ExecutionTermination, Failure, - Function, Intrinsics, Params, RemotingOptions, Rule, Scheduler, Session, Tasks, Types, Value, + Function, Intrinsics, LocalStoreOptions, Params, RemotingOptions, Rule, Scheduler, Session, + Tasks, Types, Value, }; mod testutil; @@ -383,13 +384,13 @@ py_module_initializer!(native_engine, |py, m| { tasks_ptr: PyTasks, types_ptr: PyTypes, build_root_buf: String, - local_store_dir_buf: String, local_execution_root_dir_buf: String, named_caches_dir_buf: String, ca_certs_path: Option, ignore_patterns: Vec, use_gitignore: bool, remoting_options: PyRemotingOptions, + local_store_options: PyLocalStoreOptions, exec_strategy_opts: PyExecutionStrategyOptions ) ), @@ -413,6 +414,7 @@ py_module_initializer!(native_engine, |py, m| { m.add_class::(py)?; m.add_class::(py)?; m.add_class::(py)?; + m.add_class::(py)?; m.add_class::(py)?; m.add_class::(py)?; m.add_class::(py)?; @@ -542,9 +544,6 @@ py_class!(class PyExecutionStrategyOptions |py| { }); // Represents configuration related to remote execution and caching. -// -// The data stored by PyRemotingOptions originally was passed directly into scheduler_create -// but has been broken out separately because the large number of options became unwieldy. py_class!(class PyRemotingOptions |py| { data options: RemotingOptions; @@ -586,6 +585,29 @@ py_class!(class PyRemotingOptions |py| { } }); +py_class!(class PyLocalStoreOptions |py| { + data options: LocalStoreOptions; + + def __new__( + _cls, + store_dir: String, + process_cache_max_size_bytes: usize, + files_max_size_bytes: usize, + directories_max_size_bytes: usize, + lease_time_millis: u64, + ) -> CPyResult { + Self::create_instance(py, + LocalStoreOptions { + store_dir: PathBuf::from(store_dir), + process_cache_max_size_bytes, + files_max_size_bytes, + directories_max_size_bytes, + lease_time: Duration::from_millis(lease_time_millis), + } + ) + } +}); + py_class!(class PySession |py| { data session: Session; def __new__(_cls, @@ -866,13 +888,13 @@ fn scheduler_create( tasks_ptr: PyTasks, types_ptr: PyTypes, build_root_buf: String, - local_store_dir_buf: String, local_execution_root_dir_buf: String, named_caches_dir_buf: String, ca_certs_path_buf: Option, ignore_patterns: Vec, use_gitignore: bool, remoting_options: PyRemotingOptions, + local_store_options: PyLocalStoreOptions, exec_strategy_opts: PyExecutionStrategyOptions, ) -> CPyResult { match fs::increase_limits() { @@ -901,10 +923,10 @@ fn scheduler_create( PathBuf::from(build_root_buf), ignore_patterns, use_gitignore, - PathBuf::from(local_store_dir_buf), PathBuf::from(local_execution_root_dir_buf), PathBuf::from(named_caches_dir_buf), ca_certs_path_buf.map(PathBuf::from), + local_store_options.options(py).clone(), remoting_options.options(py).clone(), exec_strategy_opts.options(py).clone(), ) diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 7f62c2312fd..9fb7654266f 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -44,7 +44,7 @@ mod session; mod tasks; mod types; -pub use crate::context::{Core, ExecutionStrategyOptions, RemotingOptions}; +pub use crate::context::{Core, ExecutionStrategyOptions, LocalStoreOptions, RemotingOptions}; pub use crate::core::{Failure, Function, Key, Params, TypeId, Value}; pub use crate::intrinsics::Intrinsics; pub use crate::scheduler::{ExecutionRequest, ExecutionTermination, Scheduler}; From c5ec6a7f36b7e835be08986392003b6fbdb7bf96 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 22 Mar 2021 16:56:22 -0700 Subject: [PATCH 2/4] Allow the file, directory, and process cache store sizes to be configured, and unify them with StoreGCService settings. [ci skip-rust] [ci skip-build-wheels] --- .../pants/engine/internals/scheduler.py | 25 ++--- .../engine/internals/scheduler_test_base.py | 5 +- src/python/pants/engine/rules_test.py | 4 +- src/python/pants/init/engine_initializer.py | 14 ++- src/python/pants/option/global_options.py | 99 ++++++++++++++++++- src/python/pants/pantsd/pants_daemon.py | 7 +- .../pants/pantsd/service/store_gc_service.py | 22 +++-- src/python/pants/testutil/rule_runner.py | 12 ++- 8 files changed, 151 insertions(+), 37 deletions(-) diff --git a/src/python/pants/engine/internals/scheduler.py b/src/python/pants/engine/internals/scheduler.py index 94c15a72473..145df59ca8f 100644 --- a/src/python/pants/engine/internals/scheduler.py +++ b/src/python/pants/engine/internals/scheduler.py @@ -57,7 +57,11 @@ ) from pants.engine.rules import Rule, RuleIndex, TaskRule from pants.engine.unions import UnionMembership, union -from pants.option.global_options import ExecutionOptions +from pants.option.global_options import ( + LOCAL_STORE_LEASE_TIME_SECS, + ExecutionOptions, + LocalStoreOptions, +) from pants.util.contextutil import temporary_file_path from pants.util.logging import LogLevel from pants.util.strutil import pluralize @@ -101,13 +105,13 @@ def __init__( ignore_patterns: List[str], use_gitignore: bool, build_root: str, - local_store_dir: str, local_execution_root_dir: str, named_caches_dir: str, ca_certs_path: Optional[str], rules: Iterable[Rule], union_membership: UnionMembership, execution_options: ExecutionOptions, + local_store_options: LocalStoreOptions, executor: PyExecutor, include_trace_on_error: bool = True, visualize_to_dir: Optional[str] = None, @@ -117,13 +121,13 @@ def __init__( :param ignore_patterns: A list of gitignore-style file patterns for pants to ignore. :param use_gitignore: If set, pay attention to .gitignore files. :param build_root: The build root as a string. - :param local_store_dir: The directory to use for storing the engine's LMDB store in. :param local_execution_root_dir: The directory to use for local execution sandboxes. :param named_caches_dir: The directory to use as the root for named mutable caches. :param ca_certs_path: Path to pem file for custom CA, if needed. :param rules: A set of Rules which is used to compute values in the graph. :param union_membership: All the registered and normalized union rules. :param execution_options: Execution options for (remote) processes. + :param local_store_options: Options for the engine's LMDB store(s). :param include_trace_on_error: Include the trace through the graph upon encountering errors. :param validate_reachability: True to assert that all rules in an otherwise successfully constructed rule graph are reachable: if a graph cannot be successfully constructed, it @@ -177,13 +181,12 @@ def __init__( execution_headers=tuple(execution_options.remote_execution_headers.items()), execution_overall_deadline_secs=execution_options.remote_execution_overall_deadline_secs, ) - # TODO: Expose options. - local_store_options = PyLocalStoreOptions( - store_dir=local_store_dir, - process_cache_max_size_bytes=(2 * 4 * 1000000000), - files_max_size_bytes=(16 * 4 * 1000000000), - directories_max_size_bytes=(2 * 4 * 1000000000), - lease_time_millis=(2 * 60 * 60 * 1000), + py_local_store_options = PyLocalStoreOptions( + store_dir=local_store_options.store_dir, + process_cache_max_size_bytes=local_store_options.processes_max_size_bytes, + files_max_size_bytes=local_store_options.files_max_size_bytes, + directories_max_size_bytes=local_store_options.directories_max_size_bytes, + lease_time_millis=LOCAL_STORE_LEASE_TIME_SECS * 1000, ) exec_stategy_opts = PyExecutionStrategyOptions( local_parallelism=execution_options.process_execution_local_parallelism, @@ -205,7 +208,7 @@ def __init__( ignore_patterns, use_gitignore, remoting_options, - local_store_options, + py_local_store_options, exec_stategy_opts, ) diff --git a/src/python/pants/engine/internals/scheduler_test_base.py b/src/python/pants/engine/internals/scheduler_test_base.py index 4b262ffaf3c..dd2ac15e0e0 100644 --- a/src/python/pants/engine/internals/scheduler_test_base.py +++ b/src/python/pants/engine/internals/scheduler_test_base.py @@ -6,7 +6,7 @@ from pants.engine.internals.native_engine import PyExecutor from pants.engine.internals.scheduler import Scheduler, SchedulerSession from pants.engine.unions import UnionMembership -from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS +from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS, DEFAULT_LOCAL_STORE_OPTIONS from pants.util.contextutil import temporary_file_path from pants.util.dirutil import safe_mkdtemp, safe_rmtree @@ -35,14 +35,12 @@ def mk_scheduler( build_root = os.path.join(work_dir, "build_root") os.makedirs(build_root) - local_store_dir = os.path.realpath(safe_mkdtemp()) local_execution_root_dir = os.path.realpath(safe_mkdtemp()) named_caches_dir = os.path.realpath(safe_mkdtemp()) scheduler = Scheduler( ignore_patterns=[], use_gitignore=False, build_root=build_root, - local_store_dir=local_store_dir, local_execution_root_dir=local_execution_root_dir, named_caches_dir=named_caches_dir, ca_certs_path=None, @@ -50,6 +48,7 @@ def mk_scheduler( union_membership=UnionMembership({}), executor=self._executor, execution_options=DEFAULT_EXECUTION_OPTIONS, + local_store_options=DEFAULT_LOCAL_STORE_OPTIONS, include_trace_on_error=include_trace_on_error, ) return scheduler.new_session( diff --git a/src/python/pants/engine/rules_test.py b/src/python/pants/engine/rules_test.py index badc71aaaea..1631c2b1f09 100644 --- a/src/python/pants/engine/rules_test.py +++ b/src/python/pants/engine/rules_test.py @@ -30,7 +30,7 @@ rule, ) from pants.engine.unions import UnionMembership -from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS +from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS, DEFAULT_LOCAL_STORE_OPTIONS from pants.testutil.rule_runner import MockGet, run_rule_with_mocks from pants.util.enums import match from pants.util.logging import LogLevel @@ -44,7 +44,6 @@ def create_scheduler(rules, validate=True): ignore_patterns=[], use_gitignore=False, build_root=str(Path.cwd()), - local_store_dir="./.pants.d/lmdb_store", local_execution_root_dir="./.pants.d", named_caches_dir="./.pants.d/named_caches", ca_certs_path=None, @@ -52,6 +51,7 @@ def create_scheduler(rules, validate=True): union_membership=UnionMembership({}), executor=_EXECUTOR, execution_options=DEFAULT_EXECUTION_OPTIONS, + local_store_options=DEFAULT_LOCAL_STORE_OPTIONS, validate_reachability=validate, ) diff --git a/src/python/pants/init/engine_initializer.py b/src/python/pants/init/engine_initializer.py index bda89926885..d83f559efd2 100644 --- a/src/python/pants/init/engine_initializer.py +++ b/src/python/pants/init/engine_initializer.py @@ -30,7 +30,12 @@ from pants.engine.target import RegisteredTargetTypes from pants.engine.unions import UnionMembership from pants.init import specs_calculator -from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS, ExecutionOptions, GlobalOptions +from pants.option.global_options import ( + DEFAULT_EXECUTION_OPTIONS, + ExecutionOptions, + GlobalOptions, + LocalStoreOptions, +) from pants.option.options_bootstrapper import OptionsBootstrapper from pants.option.subsystem import Subsystem from pants.util.ordered_set import FrozenOrderedSet @@ -175,13 +180,14 @@ def setup_graph( assert bootstrap_options is not None executor = executor or GlobalOptions.create_py_executor(bootstrap_options) execution_options = ExecutionOptions.from_options(options, env, local_only=local_only) + local_store_options = LocalStoreOptions.from_options(bootstrap_options) return EngineInitializer.setup_graph_extended( build_configuration, execution_options, executor=executor, pants_ignore_patterns=GlobalOptions.compute_pants_ignore(build_root, bootstrap_options), use_gitignore=bootstrap_options.pants_ignore_use_gitignore, - local_store_dir=bootstrap_options.local_store_dir, + local_store_options=local_store_options, local_execution_root_dir=bootstrap_options.local_execution_root_dir, named_caches_dir=bootstrap_options.named_caches_dir, ca_certs_path=bootstrap_options.ca_certs_path, @@ -198,7 +204,7 @@ def setup_graph_extended( executor: PyExecutor, pants_ignore_patterns: List[str], use_gitignore: bool, - local_store_dir: str, + local_store_options: LocalStoreOptions, local_execution_root_dir: str, named_caches_dir: str, ca_certs_path: Optional[str] = None, @@ -280,7 +286,6 @@ def ensure_optional_absolute_path(v: Optional[str]) -> Optional[str]: ignore_patterns=pants_ignore_patterns, use_gitignore=use_gitignore, build_root=build_root, - local_store_dir=ensure_absolute_path(local_store_dir), local_execution_root_dir=ensure_absolute_path(local_execution_root_dir), named_caches_dir=ensure_absolute_path(named_caches_dir), ca_certs_path=ensure_optional_absolute_path(ca_certs_path), @@ -288,6 +293,7 @@ def ensure_optional_absolute_path(v: Optional[str]) -> Optional[str]: union_membership=union_membership, executor=executor, execution_options=execution_options, + local_store_options=local_store_options, include_trace_on_error=include_trace_on_error, visualize_to_dir=native_engine_visualize_to, ) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 52fea18fafd..5d178b0e2e8 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -251,11 +251,59 @@ def from_options( ) +@dataclass(frozen=True) +class LocalStoreOptions: + """A collection of all options related to the local store. + + TODO: These options should move to a Subsystem once we add support for "bootstrap" Subsystems (ie, + allowing Subsystems to be consumed before the Scheduler has been created). + """ + + store_dir: str + processes_max_size_bytes: int + files_max_size_bytes: int + directories_max_size_bytes: int + + def target_total_size_bytes(self) -> int: + """Returns the target total size of all of the stores. + + The `max_size` values are caps on the total size of each store: the "target" size + is the size that garbage collection will attempt to shrink the stores to each time + it runs. + + NB: This value is not currently configurable, but that could be desirable in the future. + """ + max_total_size_bytes = ( + self.processes_max_size_bytes + + self.files_max_size_bytes + + self.directories_max_size_bytes + ) + return max_total_size_bytes // 10 + + @classmethod + def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: + return cls( + store_dir=Path(options.local_store_dir).resolve().as_posix(), + processes_max_size_bytes=options.local_store_processes_max_size_bytes, + files_max_size_bytes=options.local_store_files_max_size_bytes, + directories_max_size_bytes=options.local_store_directories_max_size_bytes, + ) + + _CPU_COUNT = ( len(os.sched_getaffinity(0)) if hasattr(os, "sched_getaffinity") else os.cpu_count() ) or 2 +# The time that leases are acquired for in the local store. Configured on the Python side +# in order to ease interaction with the StoreGCService, which needs to be aware of its value. +LOCAL_STORE_LEASE_TIME_SECS = 2 * 60 * 60 + + +MEGABYTES = 1000000 +GIGABYTES = 1000 * MEGABYTES + + DEFAULT_EXECUTION_OPTIONS = ExecutionOptions( # Remote execution strategy. remote_execution=False, @@ -285,6 +333,13 @@ def from_options( remote_execution_overall_deadline_secs=60 * 60, # one hour ) +DEFAULT_LOCAL_STORE_OPTIONS = LocalStoreOptions( + store_dir=os.path.join(get_pants_cachedir(), "lmdb_store"), + processes_max_size_bytes=(2 * 4 * GIGABYTES), + files_max_size_bytes=(16 * 4 * GIGABYTES), + directories_max_size_bytes=(2 * 4 * GIGABYTES), +) + class GlobalOptions(Subsystem): options_scope = GLOBAL_SCOPE @@ -701,12 +756,13 @@ def register_bootstrap_options(cls, register): ), ) + local_store_dir_flag = "--local-store-dir" cache_instructions = ( "The path may be absolute or relative. If the directory is within the build root, be " "sure to include it in `--pants-ignore`." ) register( - "--local-store-dir", + local_store_dir_flag, advanced=True, help=( f"Directory to use for the local file store, which stores the results of " @@ -715,7 +771,46 @@ def register_bootstrap_options(cls, register): # This default is also hard-coded into the engine's rust code in # fs::Store::default_path so that tools using a Store outside of pants # are likely to be able to use the same storage location. - default=os.path.join(get_pants_cachedir(), "lmdb_store"), + default=DEFAULT_LOCAL_STORE_OPTIONS.store_dir, + ) + register( + "--local-store-processes-max-size-bytes", + type=int, + advanced=True, + help=( + "The maximum size in bytes of the local store containing process cache entries. " + f"Stored below `{local_store_dir_flag}`." + ), + default=DEFAULT_LOCAL_STORE_OPTIONS.processes_max_size_bytes, + ) + register( + "--local-store-files-max-size-bytes", + type=int, + advanced=True, + help=( + "The maximum size in bytes of the local store containing files. " + f"Stored below `{local_store_dir_flag}`." + "\n\n" + "NB: This size value bounds the total size of all files, but (due to sharding of the " + "store on disk) it also bounds the per-file size to (VALUE / 16)." + "\n\n" + "This value doesn't reflect space allocated on disk, or RAM allocated (it " + "may be reflected in VIRT but not RSS). However, the default is lower than you " + "might otherwise choose because macOS creates core dumps that include MMAP'd " + "pages, and setting this too high might cause core dumps to use an unreasonable " + "amount of disk if they are enabled." + ), + default=DEFAULT_LOCAL_STORE_OPTIONS.files_max_size_bytes, + ) + register( + "--local-store-directories-max-size-bytes", + type=int, + advanced=True, + help=( + "The maximum size in bytes of the local store containing directories. " + f"Stored below `{local_store_dir_flag}`." + ), + default=DEFAULT_LOCAL_STORE_OPTIONS.directories_max_size_bytes, ) register( "--named-caches-dir", diff --git a/src/python/pants/pantsd/pants_daemon.py b/src/python/pants/pantsd/pants_daemon.py index 42795180f64..03a5a1a9bb2 100644 --- a/src/python/pants/pantsd/pants_daemon.py +++ b/src/python/pants/pantsd/pants_daemon.py @@ -21,7 +21,7 @@ from pants.init.engine_initializer import GraphScheduler from pants.init.logging import initialize_stdio, pants_log_path from pants.init.util import init_workdir -from pants.option.global_options import GlobalOptions +from pants.option.global_options import GlobalOptions, LocalStoreOptions from pants.option.option_value_container import OptionValueContainer from pants.option.options import Options from pants.option.options_bootstrapper import OptionsBootstrapper @@ -102,7 +102,10 @@ def _setup_services( max_memory_usage_in_bytes=bootstrap_options.pantsd_max_memory_usage, ) - store_gc_service = StoreGCService(graph_scheduler.scheduler) + store_gc_service = StoreGCService( + graph_scheduler.scheduler, + local_store_options=LocalStoreOptions.from_options(bootstrap_options), + ) return PantsServices(services=(scheduler_service, store_gc_service)) def __init__( diff --git a/src/python/pants/pantsd/service/store_gc_service.py b/src/python/pants/pantsd/service/store_gc_service.py index b289a5206e1..cf63a92e865 100644 --- a/src/python/pants/pantsd/service/store_gc_service.py +++ b/src/python/pants/pantsd/service/store_gc_service.py @@ -1,10 +1,17 @@ # Copyright 2018 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +from __future__ import annotations + import logging import time from pants.engine.internals.scheduler import Scheduler +from pants.option.global_options import ( + DEFAULT_LOCAL_STORE_OPTIONS, + LOCAL_STORE_LEASE_TIME_SECS, + LocalStoreOptions, +) from pants.pantsd.service.pants_service import PantsService @@ -14,18 +21,17 @@ class StoreGCService(PantsService): This service both ensures that in-use files continue to be present in the engine's Store, and performs occasional garbage collection to bound the size of the engine's Store. - NB: The lease extension interval should be significantly less than the rust-side - sharded_lmdb::DEFAULT_LEASE_TIME to ensure that valid leases are extended well before they - might expire. + NB: The lease extension interval should be a small multiple of LOCAL_STORE_LEASE_TIME_SECS + to ensure that valid leases are extended well before they might expire. """ def __init__( self, scheduler: Scheduler, - period_secs=10, - lease_extension_interval_secs=(15 * 60), - gc_interval_secs=(1 * 60 * 60), - target_size_bytes=(4 * 1024 * 1024 * 1024), + period_secs: float = 10, + lease_extension_interval_secs: float = (float(LOCAL_STORE_LEASE_TIME_SECS) / 100), + gc_interval_secs: float = (1 * 60 * 60), + local_store_options: LocalStoreOptions = DEFAULT_LOCAL_STORE_OPTIONS, ): super().__init__() self._scheduler_session = scheduler.new_session(build_id="store_gc_service_session") @@ -34,7 +40,7 @@ def __init__( self._period_secs = period_secs self._lease_extension_interval_secs = lease_extension_interval_secs self._gc_interval_secs = gc_interval_secs - self._target_size_bytes = target_size_bytes + self._target_size_bytes = local_store_options.target_total_size_bytes() self._set_next_gc() self._set_next_lease_extension() diff --git a/src/python/pants/testutil/rule_runner.py b/src/python/pants/testutil/rule_runner.py index a62f365b451..79e1de5b703 100644 --- a/src/python/pants/testutil/rule_runner.py +++ b/src/python/pants/testutil/rule_runner.py @@ -3,6 +3,7 @@ from __future__ import annotations +import dataclasses import multiprocessing import os import sys @@ -37,7 +38,7 @@ from pants.engine.unions import UnionMembership from pants.init.engine_initializer import EngineInitializer from pants.init.logging import initialize_stdio, stdio_destination -from pants.option.global_options import ExecutionOptions, GlobalOptions +from pants.option.global_options import ExecutionOptions, GlobalOptions, LocalStoreOptions from pants.option.options_bootstrapper import OptionsBootstrapper from pants.source import source_root from pants.testutil.option_util import create_options_bootstrapper @@ -141,14 +142,15 @@ def __init__( options = self.options_bootstrapper.full_options(self.build_config) global_options = self.options_bootstrapper.bootstrap_options.for_global_scope() - local_store_dir = global_options.local_store_dir + local_store_options = LocalStoreOptions.from_options(global_options) if isolated_local_store: if root_dir: lmdb_store_dir = root_dir / "lmdb_store" lmdb_store_dir.mkdir() - local_store_dir = str(lmdb_store_dir) + store_dir = str(lmdb_store_dir) else: - local_store_dir = safe_mkdtemp(prefix="lmdb_store.") + store_dir = safe_mkdtemp(prefix="lmdb_store.") + local_store_options = dataclasses.replace(local_store_options, store_dir=store_dir) local_execution_root_dir = global_options.local_execution_root_dir named_caches_dir = global_options.named_caches_dir @@ -158,7 +160,7 @@ def __init__( self.build_root, global_options ), use_gitignore=False, - local_store_dir=local_store_dir, + local_store_options=local_store_options, local_execution_root_dir=local_execution_root_dir, named_caches_dir=named_caches_dir, build_root=self.build_root, From f1fb18b509cbd44609d459f4aa4105bae0c7539f Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Mon, 22 Mar 2021 18:31:17 -0700 Subject: [PATCH 3/4] Adjust the default store sizes. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/option/global_options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 5d178b0e2e8..abd909db160 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -335,9 +335,9 @@ def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: DEFAULT_LOCAL_STORE_OPTIONS = LocalStoreOptions( store_dir=os.path.join(get_pants_cachedir(), "lmdb_store"), - processes_max_size_bytes=(2 * 4 * GIGABYTES), - files_max_size_bytes=(16 * 4 * GIGABYTES), - directories_max_size_bytes=(2 * 4 * GIGABYTES), + processes_max_size_bytes=(4 * GIGABYTES), + files_max_size_bytes=(128 * GIGABYTES), + directories_max_size_bytes=(4 * GIGABYTES), ) From d8f566345bd08ef07da5fdabd6ce37b3a6e611cb Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 23 Mar 2021 08:50:14 -0700 Subject: [PATCH 4/4] Review feedback. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/option/global_options.py | 45 ++++++++++------------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index abd909db160..5977be2ac6b 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -37,6 +37,20 @@ logger = logging.getLogger(__name__) +_CPU_COUNT = ( + len(os.sched_getaffinity(0)) if hasattr(os, "sched_getaffinity") else os.cpu_count() +) or 2 + + +# The time that leases are acquired for in the local store. Configured on the Python side +# in order to ease interaction with the StoreGCService, which needs to be aware of its value. +LOCAL_STORE_LEASE_TIME_SECS = 2 * 60 * 60 + + +MEGABYTES = 1_000_000 +GIGABYTES = 1_000 * MEGABYTES + + class GlobMatchErrorBehavior(Enum): """Describe the action to perform when matching globs in BUILD files to source files. @@ -259,10 +273,10 @@ class LocalStoreOptions: allowing Subsystems to be consumed before the Scheduler has been created). """ - store_dir: str - processes_max_size_bytes: int - files_max_size_bytes: int - directories_max_size_bytes: int + store_dir: str = os.path.join(get_pants_cachedir(), "lmdb_store") + processes_max_size_bytes: int = 16 * GIGABYTES + files_max_size_bytes: int = 256 * GIGABYTES + directories_max_size_bytes: int = 16 * GIGABYTES def target_total_size_bytes(self) -> int: """Returns the target total size of all of the stores. @@ -283,27 +297,13 @@ def target_total_size_bytes(self) -> int: @classmethod def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: return cls( - store_dir=Path(options.local_store_dir).resolve().as_posix(), + store_dir=str(Path(options.local_store_dir).resolve()), processes_max_size_bytes=options.local_store_processes_max_size_bytes, files_max_size_bytes=options.local_store_files_max_size_bytes, directories_max_size_bytes=options.local_store_directories_max_size_bytes, ) -_CPU_COUNT = ( - len(os.sched_getaffinity(0)) if hasattr(os, "sched_getaffinity") else os.cpu_count() -) or 2 - - -# The time that leases are acquired for in the local store. Configured on the Python side -# in order to ease interaction with the StoreGCService, which needs to be aware of its value. -LOCAL_STORE_LEASE_TIME_SECS = 2 * 60 * 60 - - -MEGABYTES = 1000000 -GIGABYTES = 1000 * MEGABYTES - - DEFAULT_EXECUTION_OPTIONS = ExecutionOptions( # Remote execution strategy. remote_execution=False, @@ -333,12 +333,7 @@ def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: remote_execution_overall_deadline_secs=60 * 60, # one hour ) -DEFAULT_LOCAL_STORE_OPTIONS = LocalStoreOptions( - store_dir=os.path.join(get_pants_cachedir(), "lmdb_store"), - processes_max_size_bytes=(4 * GIGABYTES), - files_max_size_bytes=(128 * GIGABYTES), - directories_max_size_bytes=(4 * GIGABYTES), -) +DEFAULT_LOCAL_STORE_OPTIONS = LocalStoreOptions() class GlobalOptions(Subsystem):