From cd11fd55af65c0055c31d9ff64005c4fea15c3ba Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sat, 26 Jan 2019 21:04:57 -0800 Subject: [PATCH 01/10] first attempt at caching local process executions - compiles - runs - may not work at all --- src/python/pants/engine/native.py | 1 + src/python/pants/engine/scheduler.py | 2 + src/python/pants/option/global_options.py | 8 + src/rust/engine/fs/src/store.rs | 205 +++++++- .../process_execution/src/cached_execution.rs | 487 ++++++++++++++++++ src/rust/engine/process_execution/src/lib.rs | 130 ++++- .../engine/process_execution/src/remote.rs | 475 ++++++----------- src/rust/engine/src/context.rs | 46 +- src/rust/engine/src/lib.rs | 9 + src/rust/engine/src/nodes.rs | 2 +- 10 files changed, 1020 insertions(+), 345 deletions(-) create mode 100644 src/rust/engine/process_execution/src/cached_execution.rs diff --git a/src/python/pants/engine/native.py b/src/python/pants/engine/native.py index 43d3331b17b..887b7c79e0e 100644 --- a/src/python/pants/engine/native.py +++ b/src/python/pants/engine/native.py @@ -740,6 +740,7 @@ def tc(constraint): self.context.utf8_buf(build_root), self.context.utf8_buf(work_dir), self.context.utf8_buf(local_store_dir), + self.context.utf8_buf(execution_options.local_execution_process_cache_namespace or ""), self.context.utf8_buf_buf(ignore_patterns), self.to_ids_buf(root_subject_types), # Remote execution config. diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 72d04e661f8..498d2c731f1 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -88,6 +88,8 @@ def __init__( self._tasks = native.new_tasks() self._register_rules(rule_index) + # TODO: we REALLY need to have a datatype for all of these so that we don't mix up arguments by + # order. self._scheduler = native.new_scheduler( tasks=self._tasks, root_subject_types=self._root_subject_types, diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 31160095c0e..e6b82b855b8 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -30,6 +30,7 @@ class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error' class ExecutionOptions(datatype([ + 'local_execution_process_cache_namespace', 'remote_store_server', 'remote_store_thread_count', 'remote_execution_server', @@ -52,6 +53,7 @@ class ExecutionOptions(datatype([ @classmethod def from_bootstrap_options(cls, bootstrap_options): return cls( + local_execution_process_cache_namespace=bootstrap_options.local_execution_process_cache_namespace, remote_store_server=bootstrap_options.remote_store_server, remote_execution_server=bootstrap_options.remote_execution_server, remote_store_thread_count=bootstrap_options.remote_store_thread_count, @@ -68,6 +70,7 @@ def from_bootstrap_options(cls, bootstrap_options): DEFAULT_EXECUTION_OPTIONS = ExecutionOptions( + local_execution_process_cache_namespace=None, remote_store_server=[], remote_store_thread_count=1, remote_execution_server=None, @@ -291,6 +294,11 @@ def register_bootstrap_options(cls, register): # This default is also hard-coded into the engine's rust code in # fs::Store::default_path default=os.path.expanduser('~/.cache/pants/lmdb_store')) + register('--local-execution-process-cache-namespace', advanced=True, + help="The cache namespace for local process execution. " + "Bump this to invalidate every artifact's local execution. " + "This is the hermetic execution equivalent of the legacy cache-key-gen-version " + "flag.") register('--remote-store-server', advanced=True, type=list, default=[], help='host:port of grpc server to use as remote execution file store.') register('--remote-store-thread-count', type=int, advanced=True, diff --git a/src/rust/engine/fs/src/store.rs b/src/rust/engine/fs/src/store.rs index 1e1f5a2fbcf..6068e4147a5 100644 --- a/src/rust/engine/fs/src/store.rs +++ b/src/rust/engine/fs/src/store.rs @@ -5,7 +5,7 @@ use boxfuture::{try_future, BoxFuture, Boxable}; use bytes::Bytes; use dirs; use futures::{future, Future}; -use hashing::Digest; +use hashing::{Digest, Fingerprint}; use protobuf::Message; use serde_derive::Serialize; use std::collections::HashMap; @@ -82,6 +82,13 @@ pub enum ShrinkBehavior { // This has the nice property that Directories can be trusted to be valid and canonical. // We may want to re-visit this if we end up wanting to handle local/remote/merged interchangably. impl Store { + pub fn fingerprint_from_bytes_unsafe(bytes: &Bytes) -> Fingerprint { + use digest::{Digest as DigestTrait, FixedOutput}; + let mut hasher = sha2::Sha256::default(); + hasher.input(&bytes); + Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()) + } + /// /// Make a store which only uses its local storage. /// @@ -232,6 +239,69 @@ impl Store { ) } + // TODO: convert every Result<_, String> into a typedef! + fn action_fingerprint( + action: bazel_protos::remote_execution::Action, + ) -> Result { + action + .write_to_bytes() + .map_err(|e| format!("Error serializing Action proto {:?}: {:?}", action, e)) + .map(|bytes| super::Store::fingerprint_from_bytes_unsafe(&Bytes::from(bytes))) + } + + /// ??? + pub fn record_process_result( + &self, + req: bazel_protos::remote_execution::Action, + res: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture<(), String> { + let converted_key_value_protos = Self::action_fingerprint(req).and_then(|req| { + res + .write_to_bytes() + .map_err(|e| format!("Error serializing ActionResult proto {:?}: {:?}", res, e)) + .map(|res| (req, Bytes::from(res))) + }); + let store = self.clone(); + future::result(converted_key_value_protos) + .and_then(move |(action_fingerprint, result_bytes)| { + store + .local + .record_process_result(action_fingerprint, result_bytes) + }) + .to_boxed() + } + + fn deserialize_action_result( + result_bytes: Bytes, + ) -> Result { + let mut action_result = bazel_protos::remote_execution::ActionResult::new(); + action_result.merge_from_bytes(&result_bytes).map_err(|e| { + format!( + "LMDB corruption: ActionResult bytes for {:?} were not valid: {:?}", + result_bytes, e + ) + })?; + Ok(action_result) + } + + /// ??? + pub fn load_process_result( + &self, + req: bazel_protos::remote_execution::Action, + ) -> BoxFuture, String> { + let store = self.clone(); + future::result(Self::action_fingerprint(req)) + .and_then(move |action_fingerprint| store.local.load_process_result(action_fingerprint)) + .and_then(|maybe_bytes| { + let deserialized_result = match maybe_bytes { + Some(bytes) => Self::deserialize_action_result(bytes).map(Some), + None => Ok(None), + }; + future::result(deserialized_result) + }) + .to_boxed() + } + /// /// Loads bytes from remote cas if required and possible (i.e. if remote is configured). Takes /// two functions f_local and f_remote. These functions are any validation or transformations you @@ -666,8 +736,7 @@ mod local { use boxfuture::{BoxFuture, Boxable}; use bytes::Bytes; - use digest::{Digest as DigestTrait, FixedOutput}; - use futures::future; + use futures::future::{self, Future}; use hashing::{Digest, Fingerprint}; use lmdb::Error::{KeyExist, NotFound}; use lmdb::{ @@ -675,7 +744,6 @@ mod local { RwTransaction, Transaction, WriteFlags, }; use log::{debug, error}; - use sha2::Sha256; use std; use std::collections::{BinaryHeap, HashMap}; use std::fmt; @@ -693,13 +761,23 @@ mod local { inner: Arc, } + struct DbEnv(Arc, Arc); + + impl DbEnv { + fn get(&self) -> (Arc, Arc) { + (Arc::clone(&self.0), Arc::clone(&self.1)) + } + } + struct InnerStore { pool: Arc, // Store directories separately from files because: // 1. They may have different lifetimes. // 2. It's nice to know whether we should be able to parse something as a proto. + // TODO: why are these `Result`s? file_dbs: Result, String>, directory_dbs: Result, String>, + process_execution_db: Result, String>, } impl ByteStore { @@ -707,11 +785,39 @@ mod local { let root = path.as_ref(); let files_root = root.join("files"); let directories_root = root.join("directories"); + let process_executions_root = root.join("process_executions"); + super::super::safe_create_dir_all(&process_executions_root).map_err(|err| { + format!( + "Error making directory for process execution store at {:?}: {:?}", + process_executions_root, err + ) + })?; + let process_executions_env = Environment::new() + .set_max_dbs(1) + .set_map_size(MAX_LOCAL_STORE_SIZE_BYTES) + // TODO: does the Environment need to be opened in a subdirectory of the db dir? see + // ShardedLmdb::make_env()! + .open(&process_executions_root) + .map_err(|err| { + format!( + "Error making process execution Environment for db at {:?}: {:?}", + process_executions_root, err + ) + })?; Ok(ByteStore { inner: Arc::new(InnerStore { pool: pool, file_dbs: ShardedLmdb::new(files_root.clone()).map(Arc::new), directory_dbs: ShardedLmdb::new(directories_root.clone()).map(Arc::new), + process_execution_db: process_executions_env + .create_db(Some("process_executions_content"), DatabaseFlags::empty()) + .map_err(|e| { + format!( + "Error creating/opening process execution content database at {:?}: {}", + process_executions_root, e + ) + }) + .map(|db| Arc::new(DbEnv(Arc::new(db), Arc::new(process_executions_env)))), }), }) } @@ -933,11 +1039,7 @@ mod local { .inner .pool .spawn_fn(move || { - let fingerprint = { - let mut hasher = Sha256::default(); - hasher.input(&bytes); - Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()) - }; + let fingerprint = super::Store::fingerprint_from_bytes_unsafe(&bytes); let digest = Digest(fingerprint, bytes.len()); let (env, content_database, lease_database) = dbs.clone()?.get(&fingerprint); @@ -1008,6 +1110,76 @@ mod local { }) }).to_boxed() } + + pub fn record_process_result( + &self, + action_fingerprint: Fingerprint, + result_bytes: Bytes, + ) -> BoxFuture<(), String> { + let db_env = self.inner.process_execution_db.clone(); + let store = self.clone(); + future::result(db_env) + .and_then(move |db_env| { + let (db, env) = db_env.get(); + store.inner.pool.spawn_fn(move || { + let put_res = env.begin_rw_txn().and_then(|mut txn| { + txn.put( + *db, + &action_fingerprint, + &result_bytes, + // TODO: this was stolen from store_bytes() -- is it still applicable? + WriteFlags::NO_OVERWRITE, + )?; + txn.commit() + }); + match put_res { + Ok(()) => Ok(()), + Err(KeyExist) => Ok(()), + Err(err) => Err(format!( + "Error storing process execution action with fingerprint {:?}: {:?}", + action_fingerprint, err + )), + } + }) + }) + .to_boxed() + .to_boxed() + } + + pub fn load_process_result( + &self, + action_fingerprint: Fingerprint, + ) -> BoxFuture, String> { + let store = self.clone(); + let db_env = store.inner.process_execution_db.clone(); + self + .inner + .pool + .spawn_fn(move || { + let (db, env) = db_env.clone()?.get(); + let ro_txn = env + .begin_ro_txn() + // TODO: is there a reason load_bytes_with() uses {} instead of {:?} here? + .map_err(|err| { + format!( + "Failed to begin read transaction for process result: {}", + err + ) + }); + ro_txn.and_then(|txn| { + let db = db.clone(); + match txn.get(*db, &action_fingerprint) { + Ok(bytes) => Ok(Some(Bytes::from(bytes))), + Err(NotFound) => Ok(None), + Err(err) => Err(format!( + "Error loading result for process execution action with fingerprint {:?}: {:?}", + action_fingerprint, err + )), + } + }) + }) + .to_boxed() + } } // Each LMDB directory can have at most one concurrent writer. @@ -1708,12 +1880,10 @@ mod remote { use bazel_protos; use boxfuture::{BoxFuture, Boxable}; use bytes::{Bytes, BytesMut}; - use digest::{Digest as DigestTrait, FixedOutput}; use futures::{self, future, Future, IntoFuture, Sink, Stream}; use grpcio; - use hashing::{Digest, Fingerprint}; + use hashing::Digest; use serverset::{Retry, Serverset}; - use sha2::Sha256; use std::cmp::min; use std::collections::HashSet; use std::sync::Arc; @@ -1831,9 +2001,7 @@ mod remote { } pub fn store_bytes(&self, bytes: Bytes) -> BoxFuture { - let mut hasher = Sha256::default(); - hasher.input(&bytes); - let fingerprint = Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()); + let fingerprint = super::Store::fingerprint_from_bytes_unsafe(&bytes); let len = bytes.len(); let digest = Digest(fingerprint, len); let resource_name = format!( @@ -2739,11 +2907,8 @@ mod tests { .write_to_bytes() .expect("Error serializing proto"), ); - let non_canonical_directory_fingerprint = { - let mut hasher = Sha256::default(); - hasher.input(&non_canonical_directory_bytes); - Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()) - }; + let non_canonical_directory_fingerprint = + super::Store::fingerprint_from_bytes_unsafe(&non_canonical_directory_bytes); let directory_digest = Digest( non_canonical_directory_fingerprint, non_canonical_directory_bytes.len(), diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs new file mode 100644 index 00000000000..c1a984231a2 --- /dev/null +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -0,0 +1,487 @@ +// Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). +// Licensed under the Apache License, Version 2.0 (see LICENSE). + +#![deny(unused_must_use)] +// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source. +#![deny( + clippy::all, + clippy::default_trait_access, + clippy::expl_impl_clone_on_copy, + clippy::if_not_else, + clippy::needless_continue, + clippy::single_match_else, + clippy::unseparated_literal_suffix, + clippy::used_underscore_binding +)] +// It is often more clear to show that nothing is being moved. +#![allow(clippy::match_ref_pats)] +// Subjective style. +#![allow( + clippy::len_without_is_empty, + clippy::redundant_field_names, + clippy::too_many_arguments +)] +// Default isn't as big a deal as people seem to think it is. +#![allow( + clippy::new_without_default, + clippy::new_without_default_derive, + clippy::new_ret_no_self +)] +// Arc can be more clear than needing to grok Orderings: +#![allow(clippy::mutex_atomic)] + +use std::collections::HashMap; +use std::path::PathBuf; + +use boxfuture::{try_future, BoxFuture, Boxable}; +use bytes::Bytes; +use futures::future::{self, Future}; +use protobuf::{self, Message as GrpcioMessage}; + +use fs::{File, PathStat}; +use hashing::Digest; + +use super::{CacheableExecuteProcessRequest, CacheableExecuteProcessResult}; + +// Environment variable which is exclusively used for cache key invalidation. +// This may be not specified in an ExecuteProcessRequest, and may be populated only by the +// CommandRunner. +const CACHE_KEY_GEN_VERSION_ENV_VAR_NAME: &str = "PANTS_CACHE_KEY_GEN_VERSION"; + +/// ???/just a neat way to separate logic as this interface evolves, not really necessary right now +pub trait SerializableProcessExecutionCodec< + // TODO: add some constraints to these? + ProcessRequest, + SerializableRequest, + ProcessResult, + SerializableResult, + ErrorType, + >: Send + Sync { + fn convert_request( + &self, + req: ProcessRequest + ) -> Result; + + fn convert_response( + &self, + res: ProcessResult, + ) -> Result; + + fn extract_response( + &self, + serializable_response: SerializableResult, + ) -> BoxFuture; +} + +#[derive(Clone)] +pub struct BazelProtosProcessExecutionCodec { + store: fs::Store, +} + +impl BazelProtosProcessExecutionCodec { + pub fn new(store: fs::Store) -> Self { + BazelProtosProcessExecutionCodec { store } + } + + fn digest_bytes(bytes: &Bytes) -> Digest { + let fingerprint = fs::Store::fingerprint_from_bytes_unsafe(&bytes); + Digest(fingerprint, bytes.len()) + } + + pub fn digest_message(message: &dyn GrpcioMessage) -> Result { + let bytes = message.write_to_bytes().map_err(|e| format!("{:?}", e))?; + Ok(Self::digest_bytes(&Bytes::from(bytes.as_slice()))) + } + + fn convert_digest(digest: Digest) -> bazel_protos::remote_execution::Digest { + let mut digest_proto = bazel_protos::remote_execution::Digest::new(); + let Digest(fingerprint, bytes_len) = digest; + digest_proto.set_hash(fingerprint.to_hex()); + digest_proto.set_size_bytes(bytes_len as i64); + digest_proto + } + + fn make_command( + req: CacheableExecuteProcessRequest, + ) -> Result { + let CacheableExecuteProcessRequest { + req, + cache_key_gen_version, + } = req; + let mut command = bazel_protos::remote_execution::Command::new(); + command.set_arguments(protobuf::RepeatedField::from_vec(req.argv.clone())); + + for (ref name, ref value) in &req.env { + if name.as_str() == CACHE_KEY_GEN_VERSION_ENV_VAR_NAME { + return Err(format!( + "Cannot set env var with name {} as that is reserved for internal use by pants", + CACHE_KEY_GEN_VERSION_ENV_VAR_NAME + )); + } + let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + env.set_name(name.to_string()); + env.set_value(value.to_string()); + command.mut_environment_variables().push(env); + } + if let Some(cache_key_gen_version) = cache_key_gen_version { + let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + env.set_name(CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_string()); + env.set_value(cache_key_gen_version.to_string()); + command.mut_environment_variables().push(env); + } + let mut output_files = req + .output_files + .iter() + .map(|p| { + p.to_str() + .map(|s| s.to_owned()) + .ok_or_else(|| format!("Non-UTF8 output file path: {:?}", p)) + }) + .collect::, String>>()?; + output_files.sort(); + command.set_output_files(protobuf::RepeatedField::from_vec(output_files)); + + let mut output_directories = req + .output_directories + .iter() + .map(|p| { + p.to_str() + .map(|s| s.to_owned()) + .ok_or_else(|| format!("Non-UTF8 output directory path: {:?}", p)) + }) + .collect::, String>>()?; + output_directories.sort(); + command.set_output_directories(protobuf::RepeatedField::from_vec(output_directories)); + + // Ideally, the JDK would be brought along as part of the input directory, but we don't currently + // have support for that. The platform with which we're experimenting for remote execution + // supports this property, and will symlink .jdk to a system-installed JDK: + // https://github.com/twitter/scoot/pull/391 + if req.jdk_home.is_some() { + command.set_platform({ + let mut platform = bazel_protos::remote_execution::Platform::new(); + platform.mut_properties().push({ + let mut property = bazel_protos::remote_execution::Platform_Property::new(); + property.set_name("JDK_SYMLINK".to_owned()); + property.set_value(".jdk".to_owned()); + property + }); + platform + }); + } + + Ok(command) + } + + pub fn make_action_with_command( + req: CacheableExecuteProcessRequest, + ) -> Result< + ( + bazel_protos::remote_execution::Action, + bazel_protos::remote_execution::Command, + ), + String, + > { + let command = Self::make_command(req.clone())?; + let mut action = bazel_protos::remote_execution::Action::new(); + action.set_command_digest((&Self::digest_message(&command)?).into()); + action.set_input_root_digest((&req.req.input_files).into()); + Ok((action, command)) + } + + fn extract_stdout( + &self, + result: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture { + if let Some(ref stdout_digest) = result.stdout_digest.into_option() { + let stdout_digest_result: Result = stdout_digest.into(); + let stdout_digest = try_future!( + stdout_digest_result.map_err(|err| format!("Error extracting stdout: {}", err)) + ); + self + .store + .load_file_bytes_with(stdout_digest, |v| v) + .map_err(move |error| { + format!( + "Error fetching stdout digest ({:?}): {:?}", + stdout_digest, error + ) + }) + .and_then(move |maybe_value| { + maybe_value.ok_or_else(|| { + format!( + "Couldn't find stdout digest ({:?}), when fetching.", + stdout_digest + ) + }) + }) + .to_boxed() + } else { + let stdout_raw = result.stdout_raw; + let stdout_copy = stdout_raw.clone(); + self + .store + .store_file_bytes(stdout_raw, true) + .map_err(move |error| format!("Error storing raw stdout: {:?}", error)) + .map(|_| stdout_copy) + .to_boxed() + } + } + + fn extract_stderr( + &self, + result: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture { + if let Some(ref stderr_digest) = result.stderr_digest.into_option() { + let stderr_digest_result: Result = stderr_digest.into(); + let stderr_digest = try_future!( + stderr_digest_result.map_err(|err| format!("Error extracting stderr: {}", err)) + ); + self + .store + .load_file_bytes_with(stderr_digest, |v| v) + .map_err(move |error| { + format!( + "Error fetching stderr digest ({:?}): {:?}", + stderr_digest, error + ) + }) + .and_then(move |maybe_value| { + maybe_value.ok_or_else(|| { + format!( + "Couldn't find stderr digest ({:?}), when fetching.", + stderr_digest + ) + }) + }) + .to_boxed() + } else { + let stderr_raw = result.stderr_raw; + let stderr_copy = stderr_raw.clone(); + self + .store + .store_file_bytes(stderr_raw, true) + .map_err(move |error| format!("Error storing raw stderr: {:?}", error)) + .map(|_| stderr_copy) + .to_boxed() + } + } + + fn extract_output_files( + &self, + result: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture { + // Get Digests of output Directories. + // Then we'll make a Directory for the output files, and merge them. + let output_directories = result.output_directories.clone(); + let mut directory_digests = Vec::with_capacity(output_directories.len() + 1); + for dir in output_directories.into_iter() { + let digest_result: Result = (&dir.tree_digest.unwrap()).into(); + let mut digest = future::done(digest_result).to_boxed(); + for component in dir.path.rsplit('/') { + let component = component.to_owned(); + let store = self.store.clone(); + digest = digest + .and_then(move |digest| { + let mut directory = bazel_protos::remote_execution::Directory::new(); + directory.mut_directories().push({ + let mut node = bazel_protos::remote_execution::DirectoryNode::new(); + node.set_name(component); + node.set_digest((&digest).into()); + node + }); + store.record_directory(&directory, true) + }) + .to_boxed(); + } + directory_digests + .push(digest.map_err(|err| format!("Error saving remote output directory: {}", err))); + } + + // Make a directory for the files + let mut path_map = HashMap::new(); + let output_files = result.output_files.clone(); + let path_stats_result: Result, String> = output_files + .into_iter() + .map(|output_file| { + let output_file_path_buf = PathBuf::from(output_file.path); + let digest = output_file + .digest + .into_option() + .ok_or_else(|| "No digest on remote execution output file".to_string())?; + let digest: Result = (&digest).into(); + path_map.insert(output_file_path_buf.clone(), digest?); + Ok(PathStat::file( + output_file_path_buf.clone(), + File { + path: output_file_path_buf, + is_executable: output_file.is_executable, + }, + )) + }) + .collect(); + + let path_stats = try_future!(path_stats_result); + + #[derive(Clone)] + struct StoreOneOffRemoteDigest { + map_of_paths_to_digests: HashMap, + } + + impl StoreOneOffRemoteDigest { + fn new(map: HashMap) -> StoreOneOffRemoteDigest { + StoreOneOffRemoteDigest { + map_of_paths_to_digests: map, + } + } + } + + impl fs::StoreFileByDigest for StoreOneOffRemoteDigest { + fn store_by_digest(&self, file: File) -> BoxFuture { + match self.map_of_paths_to_digests.get(&file.path) { + Some(digest) => future::ok(*digest), + None => future::err(format!( + "Didn't know digest for path in remote execution response: {:?}", + file.path + )), + } + .to_boxed() + } + } + + let store = self.store.clone(); + fs::Snapshot::digest_from_path_stats( + self.store.clone(), + &StoreOneOffRemoteDigest::new(path_map), + &path_stats, + ) + .map_err(move |error| { + format!( + "Error when storing the output file directory info in the remote CAS: {:?}", + error + ) + }) + .join(future::join_all(directory_digests)) + .and_then(|(files_digest, mut directory_digests)| { + directory_digests.push(files_digest); + fs::Snapshot::merge_directories(store, directory_digests) + .map_err(|err| format!("Error when merging output files and directories: {}", err)) + }) + .to_boxed() + } +} + +impl + SerializableProcessExecutionCodec< + CacheableExecuteProcessRequest, + bazel_protos::remote_execution::Action, + CacheableExecuteProcessResult, + bazel_protos::remote_execution::ActionResult, + // TODO: better error type? + String, + > for BazelProtosProcessExecutionCodec +{ + fn convert_request( + &self, + req: CacheableExecuteProcessRequest, + ) -> Result { + let (action, _) = Self::make_action_with_command(req)?; + Ok(action) + } + + fn convert_response( + &self, + res: CacheableExecuteProcessResult, + ) -> Result { + let mut action_proto = bazel_protos::remote_execution::ActionResult::new(); + let mut output_directory = bazel_protos::remote_execution::OutputDirectory::new(); + let output_directory_digest = Self::convert_digest(res.output_directory); + output_directory.set_tree_digest(output_directory_digest); + action_proto.set_output_directories(protobuf::RepeatedField::from_vec(vec![output_directory])); + action_proto.set_exit_code(res.exit_code); + action_proto.set_stdout_raw(res.stdout.clone()); + action_proto.set_stdout_digest(Self::convert_digest(Self::digest_bytes(&res.stdout))); + action_proto.set_stderr_raw(res.stderr.clone()); + action_proto.set_stderr_digest(Self::convert_digest(Self::digest_bytes(&res.stderr))); + Ok(action_proto) + } + + fn extract_response( + &self, + res: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture { + self + .extract_stdout(res.clone()) + .join(self.extract_stderr(res.clone())) + .join(self.extract_output_files(res.clone())) + .map( + move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { + stdout, + stderr, + exit_code: res.exit_code, + output_directory, + }, + ) + .to_boxed() + } +} + +/// ???/it's called "immediate" because it's a best-effort thing located locally (???) +pub trait ImmediateExecutionCache: Send + Sync { + fn record_process_result(&self, req: ProcessRequest, res: ProcessResult) + -> BoxFuture<(), String>; + + fn load_process_result(&self, req: ProcessRequest) -> BoxFuture, String>; +} + +/// ??? +#[derive(Clone)] +pub struct ActionCache { + store: fs::Store, + process_execution_codec: BazelProtosProcessExecutionCodec, +} + +impl ActionCache { + pub fn new(store: fs::Store) -> Self { + ActionCache { + store: store.clone(), + process_execution_codec: BazelProtosProcessExecutionCodec::new(store), + } + } +} + +impl ImmediateExecutionCache + for ActionCache +{ + fn record_process_result( + &self, + req: CacheableExecuteProcessRequest, + res: CacheableExecuteProcessResult, + ) -> BoxFuture<(), String> { + let codec = self.process_execution_codec.clone(); + let converted_key_value_protos = codec + .convert_request(req) + .and_then(|req| codec.convert_response(res).map(|res| (req, res))); + let store = self.store.clone(); + future::result(converted_key_value_protos) + .and_then(move |(action_request, action_result)| { + store.record_process_result(action_request, action_result) + }) + .to_boxed() + } + + fn load_process_result( + &self, + req: CacheableExecuteProcessRequest, + ) -> BoxFuture, String> { + let codec = self.process_execution_codec.clone(); + let store = self.store.clone(); + future::result(codec.convert_request(req)) + .and_then(move |action_proto| store.load_process_result(action_proto)) + .and_then(move |maybe_action_result| match maybe_action_result { + Some(action_result) => codec.extract_response(action_result).map(Some).to_boxed(), + None => future::result(Ok(None)).to_boxed(), + }) + .to_boxed() + } +} diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 5f71fe0f231..2be7d4cad9a 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -30,8 +30,9 @@ // Arc can be more clear than needing to grok Orderings: #![allow(clippy::mutex_atomic)] -use boxfuture::BoxFuture; +use boxfuture::{BoxFuture, Boxable}; use bytes::Bytes; +use futures::future::{self, Future}; use std::collections::{BTreeMap, BTreeSet}; use std::ops::AddAssign; use std::path::PathBuf; @@ -40,8 +41,13 @@ use std::time::Duration; use async_semaphore::AsyncSemaphore; +pub mod cached_execution; pub mod local; pub mod remote; +pub use crate::cached_execution::{ + ActionCache, BazelProtosProcessExecutionCodec, ImmediateExecutionCache, + SerializableProcessExecutionCodec, +}; /// /// A process to be executed. @@ -86,6 +92,23 @@ pub struct ExecuteProcessRequest { pub jdk_home: Option, } +/// ???/DON'T LET THE `cache_key_gen_version` BECOME A KITCHEN SINK!!! +#[derive(Clone)] +pub struct CacheableExecuteProcessRequest { + req: ExecuteProcessRequest, + // TODO: give this a better type than Option (everywhere)! + cache_key_gen_version: Option, +} + +impl CacheableExecuteProcessRequest { + fn new(req: ExecuteProcessRequest, cache_key_gen_version: Option) -> Self { + CacheableExecuteProcessRequest { + req, + cache_key_gen_version, + } + } +} + /// /// The result of running a process. /// @@ -102,6 +125,41 @@ pub struct FallibleExecuteProcessResult { pub execution_attempts: Vec, } +impl FallibleExecuteProcessResult { + fn without_execution_attempts(&self) -> CacheableExecuteProcessResult { + CacheableExecuteProcessResult { + stdout: self.stdout.clone(), + stderr: self.stderr.clone(), + exit_code: self.exit_code, + output_directory: self.output_directory, + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct CacheableExecuteProcessResult { + pub stdout: Bytes, + pub stderr: Bytes, + pub exit_code: i32, + pub output_directory: hashing::Digest, +} + +impl CacheableExecuteProcessResult { + fn with_execution_attempts( + &self, + execution_attempts: Vec, + ) -> FallibleExecuteProcessResult { + FallibleExecuteProcessResult { + stdout: self.stdout.clone(), + stderr: self.stderr.clone(), + exit_code: self.exit_code, + output_directory: self.output_directory, + execution_attempts, + } + } +} + +// TODO: remove this method! #[cfg(test)] impl FallibleExecuteProcessResult { pub fn without_execution_attempts(mut self) -> Self { @@ -156,3 +214,73 @@ impl CommandRunner for BoundedCommandRunner { self.inner.1.with_acquired(move || inner.0.run(req)) } } + +/// +/// A CommandRunner wrapper that attempts to cache process executions. +/// +#[derive(Clone)] +pub struct CachingCommandRunner { + inner: Arc>, + cache: Arc< + Box>, + >, + cache_key_gen_version: Option, +} + +impl CachingCommandRunner { + pub fn from_store( + inner: Box, + store: fs::Store, + cache_key_gen_version: Option, + ) -> Self { + let action_cache = ActionCache::new(store); + let boxed_cache = Box::new(action_cache) + as Box< + dyn ImmediateExecutionCache, + >; + Self::new(inner, boxed_cache, cache_key_gen_version) + } + + pub fn new( + inner: Box, + cache: Box< + dyn ImmediateExecutionCache, + >, + cache_key_gen_version: Option, + ) -> Self { + CachingCommandRunner { + inner: Arc::new(inner), + cache: Arc::new(cache), + cache_key_gen_version, + } + } +} + +impl CommandRunner for CachingCommandRunner { + fn run(&self, req: ExecuteProcessRequest) -> BoxFuture { + let cacheable_request = + CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); + let cache = self.cache.clone(); + let inner = self.inner.clone(); + cache + .load_process_result(cacheable_request.clone()) + .and_then(move |cache_fetch| match cache_fetch { + // We have a cache hit! + Some(cached_execution_result) => future::result(Ok(cached_execution_result)).to_boxed(), + // We have to actually run the process now. + None => inner + .run(req) + .and_then(move |res| { + let cacheable_process_result = res.without_execution_attempts(); + cache + .record_process_result(cacheable_request, cacheable_process_result.clone()) + .map(|()| cacheable_process_result) + }) + .to_boxed(), + }) + // NB: We clear metadata about execution attempts when returning a cacheable process execution + // request. + .map(|cacheable_process_result| cacheable_process_result.with_execution_attempts(vec![])) + .to_boxed() + } +} diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 930c6adb79a..aa589799915 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -1,12 +1,9 @@ -use std::collections::HashMap; -use std::path::PathBuf; use std::time::{Duration, Instant}; use bazel_protos; use boxfuture::{try_future, BoxFuture, Boxable}; use bytes::Bytes; -use digest::{Digest as DigestTrait, FixedOutput}; -use fs::{self, File, PathStat, Store}; +use fs::{self, Store}; use futures::{future, Future, Stream}; use futures_timer::Delay; use hashing::{Digest, Fingerprint}; @@ -14,10 +11,13 @@ use log::{debug, trace, warn}; use parking_lot::Mutex; use prost::Message; use protobuf::{self, Message as GrpcioMessage, ProtobufEnum}; -use sha2::Sha256; use time; -use super::{ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult}; +use super::{ + BazelProtosProcessExecutionCodec, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, + ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult, + SerializableProcessExecutionCodec, +}; use std; use std::cmp::min; @@ -29,11 +29,6 @@ use tower_grpc::Request; use tower_h2::client; use tower_util::MakeService; -// Environment variable which is exclusively used for cache key invalidation. -// This may be not specified in an ExecuteProcessRequest, and may be populated only by the -// CommandRunner. -const CACHE_KEY_GEN_VERSION_ENV_VAR_NAME: &str = "PANTS_CACHE_KEY_GEN_VERSION"; - #[derive(Debug)] enum OperationOrStatus { Operation(bazel_protos::google::longrunning::Operation), @@ -59,6 +54,7 @@ pub struct CommandRunner { clients: futures::future::Shared>, store: Store, futures_timer_thread: resettable::Resettable, + process_execution_codec: BazelProtosProcessExecutionCodecV2, } #[derive(Debug, PartialEq)] @@ -121,6 +117,138 @@ impl CommandRunner { } } +#[derive(Clone)] +pub struct BazelProcessExecutionRequestV2 { + action: bazel_protos::remote_execution::Action, + command: bazel_protos::remote_execution::Command, + execute_request: bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, +} + +#[derive(Clone)] +pub struct BazelProtosProcessExecutionCodecV2 { + inner: BazelProtosProcessExecutionCodec, + instance_name: Option, +} + +impl BazelProtosProcessExecutionCodecV2 { + fn new(store: fs::Store, instance_name: Option) -> Self { + let inner = BazelProtosProcessExecutionCodec::new(store); + BazelProtosProcessExecutionCodecV2 { + inner, + instance_name, + } + } + + fn convert_digest( + digest: bazel_protos::build::bazel::remote::execution::v2::Digest, + ) -> bazel_protos::remote_execution::Digest { + let mut resulting_digest = bazel_protos::remote_execution::Digest::new(); + resulting_digest.set_hash(digest.hash); + resulting_digest.set_size_bytes(digest.size_bytes); + resulting_digest + } + + fn convert_output_file( + output_file: bazel_protos::build::bazel::remote::execution::v2::OutputFile, + ) -> bazel_protos::remote_execution::OutputFile { + let mut resulting_output_file = bazel_protos::remote_execution::OutputFile::new(); + resulting_output_file.set_path(output_file.path); + if let Some(digest) = output_file.digest.map(Self::convert_digest) { + resulting_output_file.set_digest(digest); + } + resulting_output_file.set_is_executable(output_file.is_executable); + resulting_output_file + } + + fn convert_output_directory( + output_dir: bazel_protos::build::bazel::remote::execution::v2::OutputDirectory, + ) -> bazel_protos::remote_execution::OutputDirectory { + let mut resulting_output_dir = bazel_protos::remote_execution::OutputDirectory::new(); + resulting_output_dir.set_path(output_dir.path); + if let Some(digest) = output_dir.tree_digest.map(Self::convert_digest) { + resulting_output_dir.set_tree_digest(digest); + } + resulting_output_dir + } + + fn convert_action_result( + action_result: bazel_protos::build::bazel::remote::execution::v2::ActionResult, + ) -> bazel_protos::remote_execution::ActionResult { + let mut resulting_action_result = bazel_protos::remote_execution::ActionResult::new(); + resulting_action_result.set_output_files(protobuf::RepeatedField::from_vec( + action_result + .output_files + .iter() + .cloned() + .map(Self::convert_output_file) + .collect::>(), + )); + resulting_action_result.set_output_directories(protobuf::RepeatedField::from_vec( + action_result + .output_directories + .iter() + .cloned() + .map(Self::convert_output_directory) + .collect(), + )); + resulting_action_result.set_exit_code(action_result.exit_code); + resulting_action_result.set_stdout_raw(Bytes::from(action_result.stdout_raw)); + if let Some(digest) = action_result.stdout_digest.map(Self::convert_digest) { + resulting_action_result.set_stdout_digest(digest); + } + resulting_action_result.set_stderr_raw(Bytes::from(action_result.stderr_raw)); + if let Some(digest) = action_result.stderr_digest.map(Self::convert_digest) { + resulting_action_result.set_stderr_digest(digest); + } + // TODO: resulting_action_result.set_execution_metadata(); + resulting_action_result + } +} + +impl + SerializableProcessExecutionCodec< + CacheableExecuteProcessRequest, + BazelProcessExecutionRequestV2, + CacheableExecuteProcessResult, + bazel_protos::build::bazel::remote::execution::v2::ActionResult, + String, + > for BazelProtosProcessExecutionCodecV2 +{ + fn convert_request( + &self, + req: CacheableExecuteProcessRequest, + ) -> Result { + let (action, command) = BazelProtosProcessExecutionCodec::make_action_with_command(req)?; + let execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { + action_digest: Some((&BazelProtosProcessExecutionCodec::digest_message(&action)?).into()), + skip_cache_lookup: false, + instance_name: self.instance_name.clone().unwrap_or_default(), + execution_policy: None, + results_cache_policy: None, + }; + Ok(BazelProcessExecutionRequestV2 { + action, + command, + execute_request, + }) + } + + fn convert_response( + &self, + _res: CacheableExecuteProcessResult, + ) -> Result { + panic!("converting from a cacheable process request to a v2 ActionResult is not yet supported"); + } + + fn extract_response( + &self, + serializable_response: bazel_protos::build::bazel::remote::execution::v2::ActionResult, + ) -> BoxFuture { + let action_result_v1 = Self::convert_action_result(serializable_response); + self.inner.extract_response(action_result_v1) + } +} + impl super::CommandRunner for CommandRunner { /// /// Runs a command via a gRPC service implementing the Bazel Remote Execution API @@ -145,8 +273,11 @@ impl super::CommandRunner for CommandRunner { let clients = self.clients.clone(); let store = self.store.clone(); - let execute_request_result = - make_execute_request(&req, &self.instance_name, &self.cache_key_gen_version); + let cacheable_execute_process_request = + CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); + let execute_request_result = self + .process_execution_codec + .convert_request(cacheable_execute_process_request); let ExecuteProcessRequest { description, @@ -158,7 +289,11 @@ impl super::CommandRunner for CommandRunner { let description2 = description.clone(); match execute_request_result { - Ok((action, command, execute_request)) => { + Ok(BazelProcessExecutionRequestV2 { + action, + command, + execute_request, + }) => { let command_runner = self.clone(); let command_runner2 = self.clone(); let execute_request2 = execute_request.clone(); @@ -385,11 +520,13 @@ impl CommandRunner { .shared(); Ok(CommandRunner { cache_key_gen_version, - instance_name, + // TODO: this may be able to be removed! + instance_name: instance_name.clone(), authorization_header: oauth_bearer_token.map(|t| format!("Bearer {}", t)), clients, - store, + store: store.clone(), futures_timer_thread, + process_execution_codec: BazelProtosProcessExecutionCodecV2::new(store, instance_name), }) } @@ -497,17 +634,14 @@ impl CommandRunner { if status.code == bazel_protos::google::rpc::Code::Ok.into() { if let Some(result) = maybe_result { return self - .extract_stdout(&result) - .join(self.extract_stderr(&result)) - .join(self.extract_output_files(&result)) - .and_then(move |((stdout, stderr), output_directory)| { - Ok(FallibleExecuteProcessResult { - stdout: stdout, - stderr: stderr, - exit_code: result.exit_code, - output_directory: output_directory, - execution_attempts: execution_attempts, - }) + .process_execution_codec + .extract_response(result.clone()) + .map(|cacheable_result| cacheable_result.with_execution_attempts(execution_attempts)) + .map_err(move |err| { + ExecutionError::Fatal(format!( + "error deocding process result {:?}: {:?}", + result, err + )) }) .to_boxed(); } else { @@ -602,281 +736,6 @@ impl CommandRunner { } .to_boxed() } - - fn extract_stdout( - &self, - result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, - ) -> BoxFuture { - if let Some(ref stdout_digest) = result.stdout_digest { - let stdout_digest_result: Result = stdout_digest.into(); - let stdout_digest = try_future!(stdout_digest_result - .map_err(|err| ExecutionError::Fatal(format!("Error extracting stdout: {}", err)))); - self - .store - .load_file_bytes_with(stdout_digest, |v| v) - .map_err(move |error| { - ExecutionError::Fatal(format!( - "Error fetching stdout digest ({:?}): {:?}", - stdout_digest, error - )) - }) - .and_then(move |maybe_value| { - maybe_value.ok_or_else(|| { - ExecutionError::Fatal(format!( - "Couldn't find stdout digest ({:?}), when fetching.", - stdout_digest - )) - }) - }) - .to_boxed() - } else { - let stdout_raw = Bytes::from(result.stdout_raw.as_slice()); - let stdout_copy = stdout_raw.clone(); - self - .store - .store_file_bytes(stdout_raw, true) - .map_err(move |error| { - ExecutionError::Fatal(format!("Error storing raw stdout: {:?}", error)) - }) - .map(|_| stdout_copy) - .to_boxed() - } - } - - fn extract_stderr( - &self, - result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, - ) -> BoxFuture { - if let Some(ref stderr_digest) = result.stderr_digest { - let stderr_digest_result: Result = stderr_digest.into(); - let stderr_digest = try_future!(stderr_digest_result - .map_err(|err| ExecutionError::Fatal(format!("Error extracting stderr: {}", err)))); - self - .store - .load_file_bytes_with(stderr_digest, |v| v) - .map_err(move |error| { - ExecutionError::Fatal(format!( - "Error fetching stderr digest ({:?}): {:?}", - stderr_digest, error - )) - }) - .and_then(move |maybe_value| { - maybe_value.ok_or_else(|| { - ExecutionError::Fatal(format!( - "Couldn't find stderr digest ({:?}), when fetching.", - stderr_digest - )) - }) - }) - .to_boxed() - } else { - let stderr_raw = Bytes::from(result.stderr_raw.as_slice()); - let stderr_copy = stderr_raw.clone(); - self - .store - .store_file_bytes(stderr_raw, true) - .map_err(move |error| { - ExecutionError::Fatal(format!("Error storing raw stderr: {:?}", error)) - }) - .map(|_| stderr_copy) - .to_boxed() - } - } - - fn extract_output_files( - &self, - result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, - ) -> BoxFuture { - // Get Digests of output Directories. - // Then we'll make a Directory for the output files, and merge them. - let output_directories = result.output_directories.clone(); - let mut directory_digests = Vec::with_capacity(output_directories.len() + 1); - for dir in output_directories { - let digest_result: Result = (&dir.tree_digest.unwrap()).into(); - let mut digest = future::done(digest_result).to_boxed(); - for component in dir.path.rsplit('/') { - let component = component.to_owned(); - let store = self.store.clone(); - digest = digest - .and_then(move |digest| { - let mut directory = bazel_protos::remote_execution::Directory::new(); - directory.mut_directories().push({ - let mut node = bazel_protos::remote_execution::DirectoryNode::new(); - node.set_name(component); - node.set_digest((&digest).into()); - node - }); - store.record_directory(&directory, true) - }) - .to_boxed(); - } - directory_digests.push(digest.map_err(|err| { - ExecutionError::Fatal(format!("Error saving remote output directory: {}", err)) - })); - } - - // Make a directory for the files - let mut path_map = HashMap::new(); - let output_files = result.output_files.clone(); - let path_stats_result: Result, String> = output_files - .into_iter() - .map(|output_file| { - let output_file_path_buf = PathBuf::from(output_file.path); - let digest = output_file - .digest - .ok_or_else(|| "No digest on remote execution output file".to_string())?; - let digest: Result = (&digest).into(); - path_map.insert(output_file_path_buf.clone(), digest?); - Ok(PathStat::file( - output_file_path_buf.clone(), - File { - path: output_file_path_buf, - is_executable: output_file.is_executable, - }, - )) - }) - .collect(); - - let path_stats = try_future!(path_stats_result.map_err(ExecutionError::Fatal)); - - #[derive(Clone)] - struct StoreOneOffRemoteDigest { - map_of_paths_to_digests: HashMap, - } - - impl StoreOneOffRemoteDigest { - fn new(map: HashMap) -> StoreOneOffRemoteDigest { - StoreOneOffRemoteDigest { - map_of_paths_to_digests: map, - } - } - } - - impl fs::StoreFileByDigest for StoreOneOffRemoteDigest { - fn store_by_digest(&self, file: File) -> BoxFuture { - match self.map_of_paths_to_digests.get(&file.path) { - Some(digest) => future::ok(*digest), - None => future::err(format!( - "Didn't know digest for path in remote execution response: {:?}", - file.path - )), - } - .to_boxed() - } - } - - let store = self.store.clone(); - fs::Snapshot::digest_from_path_stats( - self.store.clone(), - &StoreOneOffRemoteDigest::new(path_map), - &path_stats, - ) - .map_err(move |error| { - ExecutionError::Fatal(format!( - "Error when storing the output file directory info in the remote CAS: {:?}", - error - )) - }) - .join(future::join_all(directory_digests)) - .and_then(|(files_digest, mut directory_digests)| { - directory_digests.push(files_digest); - fs::Snapshot::merge_directories(store, directory_digests).map_err(|err| { - ExecutionError::Fatal(format!( - "Error when merging output files and directories: {}", - err - )) - }) - }) - .to_boxed() - } -} - -fn make_execute_request( - req: &ExecuteProcessRequest, - instance_name: &Option, - cache_key_gen_version: &Option, -) -> Result< - ( - bazel_protos::remote_execution::Action, - bazel_protos::remote_execution::Command, - bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, - ), - String, -> { - let mut command = bazel_protos::remote_execution::Command::new(); - command.set_arguments(protobuf::RepeatedField::from_vec(req.argv.clone())); - for (ref name, ref value) in &req.env { - if name.as_str() == CACHE_KEY_GEN_VERSION_ENV_VAR_NAME { - return Err(format!( - "Cannot set env var with name {} as that is reserved for internal use by pants", - CACHE_KEY_GEN_VERSION_ENV_VAR_NAME - )); - } - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); - env.set_name(name.to_string()); - env.set_value(value.to_string()); - command.mut_environment_variables().push(env); - } - if let Some(cache_key_gen_version) = cache_key_gen_version { - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); - env.set_name(CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_string()); - env.set_value(cache_key_gen_version.to_string()); - command.mut_environment_variables().push(env); - } - let mut output_files = req - .output_files - .iter() - .map(|p| { - p.to_str() - .map(|s| s.to_owned()) - .ok_or_else(|| format!("Non-UTF8 output file path: {:?}", p)) - }) - .collect::, String>>()?; - output_files.sort(); - command.set_output_files(protobuf::RepeatedField::from_vec(output_files)); - - let mut output_directories = req - .output_directories - .iter() - .map(|p| { - p.to_str() - .map(|s| s.to_owned()) - .ok_or_else(|| format!("Non-UTF8 output directory path: {:?}", p)) - }) - .collect::, String>>()?; - output_directories.sort(); - command.set_output_directories(protobuf::RepeatedField::from_vec(output_directories)); - - // Ideally, the JDK would be brought along as part of the input directory, but we don't currently - // have support for that. The platform with which we're experimenting for remote execution - // supports this property, and will symlink .jdk to a system-installed JDK: - // https://github.com/twitter/scoot/pull/391 - if req.jdk_home.is_some() { - command.set_platform({ - let mut platform = bazel_protos::remote_execution::Platform::new(); - platform.mut_properties().push({ - let mut property = bazel_protos::remote_execution::Platform_Property::new(); - property.set_name("JDK_SYMLINK".to_owned()); - property.set_value(".jdk".to_owned()); - property - }); - platform - }); - } - - let mut action = bazel_protos::remote_execution::Action::new(); - action.set_command_digest((&digest(&command)?).into()); - action.set_input_root_digest((&req.input_files).into()); - - let execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some((&digest(&action)?).into()), - skip_cache_lookup: false, - instance_name: instance_name.clone().unwrap_or_default(), - execution_policy: None, - results_cache_policy: None, - }; - - Ok((action, command, execute_request)) } fn format_error(error: &bazel_protos::google::rpc::Status) -> String { @@ -926,18 +785,6 @@ fn towergrpcerror_to_string(error: tower_grpc::Error) -> } } -fn digest(message: &dyn GrpcioMessage) -> Result { - let bytes = message.write_to_bytes().map_err(|e| format!("{:?}", e))?; - - let mut hasher = Sha256::default(); - hasher.input(&bytes); - - Ok(Digest( - Fingerprint::from_bytes_unsafe(&hasher.fixed_result()), - bytes.len(), - )) -} - fn timespec_from(timestamp: &Option) -> time::Timespec { if let Some(timestamp) = timestamp { time::Timespec::new(timestamp.seconds, timestamp.nanos) diff --git a/src/rust/engine/src/context.rs b/src/rust/engine/src/context.rs index 1fac707784c..6dc120fae69 100644 --- a/src/rust/engine/src/context.rs +++ b/src/rust/engine/src/context.rs @@ -21,11 +21,19 @@ use boxfuture::{BoxFuture, Boxable}; use fs::{self, safe_create_dir_all_ioerror, PosixFS, ResettablePool, Store}; use graph::{EntryId, Graph, NodeContext}; use log::debug; -use process_execution::{self, BoundedCommandRunner, CommandRunner}; +use process_execution::{self, BoundedCommandRunner, CachingCommandRunner, CommandRunner}; use rand::seq::SliceRandom; use reqwest; use resettable::Resettable; +// TODO: better name!!! +#[derive(Clone)] +struct StoreAndCommandRunnerAndHttpClient { + store: Store, + command_runner: Arc>, + http_client: reqwest::r#async::Client, +} + /// /// The core context shared (via Arc) between the Scheduler and the Context objects of /// all running Nodes. @@ -42,8 +50,7 @@ pub struct Core { pub fs_pool: Arc, pub runtime: Resettable>, pub futures_timer_thread: Resettable, - store_and_command_runner_and_http_client: - Resettable<(Store, BoundedCommandRunner, reqwest::r#async::Client)>, + store_and_command_runner_and_http_client: Resettable, pub vfs: PosixFS, pub build_root: PathBuf, } @@ -57,6 +64,8 @@ impl Core { ignore_patterns: &[String], work_dir: PathBuf, local_store_dir: PathBuf, + // TODO: REALLY give this a better type than Option!!! + local_execution_process_cache_namespace: Option, remote_store_servers: Vec, remote_execution_server: Option, remote_execution_process_cache_namespace: Option, @@ -129,6 +138,7 @@ impl Core { }) .unwrap_or_else(|e| panic!("Could not initialize Store: {:?}", e)); + // NOTE: This is where the process execution mechanism is selected! let underlying_command_runner: Box = match &remote_execution_server { Some(ref address) => Box::new( process_execution::remote::CommandRunner::new( @@ -149,12 +159,24 @@ impl Core { )) as Box, }; - let command_runner = + let bounded_command_runner = BoundedCommandRunner::new(underlying_command_runner, process_execution_parallelism); + let caching_command_runner = CachingCommandRunner::from_store( + Box::new(bounded_command_runner) as Box, + store.clone(), + local_execution_process_cache_namespace.clone(), + ); + + let command_runner = Arc::new(Box::new(caching_command_runner) as Box); + let http_client = reqwest::r#async::Client::new(); - (store, command_runner, http_client) + StoreAndCommandRunnerAndHttpClient { + store, + command_runner, + http_client, + } }); let rule_graph = RuleGraph::new(&tasks, root_subject_types); @@ -211,15 +233,21 @@ impl Core { } pub fn store(&self) -> Store { - self.store_and_command_runner_and_http_client.get().0 + self.store_and_command_runner_and_http_client.get().store } - pub fn command_runner(&self) -> BoundedCommandRunner { - self.store_and_command_runner_and_http_client.get().1 + pub fn command_runner(&self) -> Arc> { + self + .store_and_command_runner_and_http_client + .get() + .command_runner } pub fn http_client(&self) -> reqwest::r#async::Client { - self.store_and_command_runner_and_http_client.get().2 + self + .store_and_command_runner_and_http_client + .get() + .http_client } } diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index a2074ee81a4..058d2db966d 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -198,6 +198,7 @@ pub extern "C" fn scheduler_create( build_root_buf: Buffer, work_dir_buf: Buffer, local_store_dir_buf: Buffer, + local_execution_process_cache_namespace: Buffer, ignore_patterns_buf: BufferBuffer, root_type_ids: TypeIdBuffer, remote_store_servers_buf: BufferBuffer, @@ -245,6 +246,9 @@ pub extern "C" fn scheduler_create( }; let mut tasks = with_tasks(tasks_ptr, |tasks| tasks.clone()); tasks.intrinsics_set(&types); + let local_execution_process_cache_namespace_string = local_execution_process_cache_namespace + .to_string() + .expect("local_execution_process_cache_namespace was not valid UTF8"); // Allocate on the heap via `Box` and return a raw pointer to the boxed value. let remote_store_servers_vec = remote_store_servers_buf .to_strings() @@ -285,6 +289,11 @@ pub extern "C" fn scheduler_create( &ignore_patterns, PathBuf::from(work_dir_buf.to_os_string()), PathBuf::from(local_store_dir_buf.to_os_string()), + if local_execution_process_cache_namespace_string.is_empty() { + None + } else { + Some(local_execution_process_cache_namespace_string) + }, remote_store_servers_vec, if remote_execution_server_string.is_empty() { None diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 3a9587e741e..fe92ed2e6ad 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -25,7 +25,7 @@ use fs::{ PathGlobs, PathStat, StoreFileByDigest, StrictGlobMatching, VFS, }; use hashing; -use process_execution::{self, CommandRunner}; +use process_execution; use graph::{Entry, Node, NodeError, NodeTracer, NodeVisualizer}; From e7e055b0b184245b68559a6b4edcd3cf147e66bd Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sat, 26 Jan 2019 23:04:33 -0800 Subject: [PATCH 02/10] move more to cached_exeuction.rs, and populate execution metadata proto --- src/rust/engine/fs/src/store.rs | 1 + .../process_execution/src/cached_execution.rs | 145 +++++++++++++++++- src/rust/engine/process_execution/src/lib.rs | 118 +------------- .../engine/process_execution/src/remote.rs | 76 ++++++++- 4 files changed, 221 insertions(+), 119 deletions(-) diff --git a/src/rust/engine/fs/src/store.rs b/src/rust/engine/fs/src/store.rs index 6068e4147a5..faa5a7539b1 100644 --- a/src/rust/engine/fs/src/store.rs +++ b/src/rust/engine/fs/src/store.rs @@ -145,6 +145,7 @@ impl Store { /// /// Store a file locally. /// + // TODO: use an enum instead of a bool, and describe in that struct what `initial_lease` means! pub fn store_file_bytes(&self, bytes: Bytes, initial_lease: bool) -> BoxFuture { self .local diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index c1a984231a2..369e3e5c67c 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -32,22 +32,64 @@ use std::collections::HashMap; use std::path::PathBuf; +use std::sync::Arc; use boxfuture::{try_future, BoxFuture, Boxable}; use bytes::Bytes; use futures::future::{self, Future}; +use log::debug; use protobuf::{self, Message as GrpcioMessage}; use fs::{File, PathStat}; use hashing::Digest; -use super::{CacheableExecuteProcessRequest, CacheableExecuteProcessResult}; +use super::{CommandRunner, ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult}; // Environment variable which is exclusively used for cache key invalidation. // This may be not specified in an ExecuteProcessRequest, and may be populated only by the // CommandRunner. const CACHE_KEY_GEN_VERSION_ENV_VAR_NAME: &str = "PANTS_CACHE_KEY_GEN_VERSION"; +/// ???/DON'T LET THE `cache_key_gen_version` BECOME A KITCHEN SINK!!! +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct CacheableExecuteProcessRequest { + req: ExecuteProcessRequest, + // TODO: give this a better type than Option (everywhere)! + cache_key_gen_version: Option, +} + +impl CacheableExecuteProcessRequest { + pub fn new(req: ExecuteProcessRequest, cache_key_gen_version: Option) -> Self { + CacheableExecuteProcessRequest { + req, + cache_key_gen_version, + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct CacheableExecuteProcessResult { + pub stdout: Bytes, + pub stderr: Bytes, + pub exit_code: i32, + pub output_directory: hashing::Digest, +} + +impl CacheableExecuteProcessResult { + pub fn with_execution_attempts( + &self, + execution_attempts: Vec, + ) -> FallibleExecuteProcessResult { + FallibleExecuteProcessResult { + stdout: self.stdout.clone(), + stderr: self.stderr.clone(), + exit_code: self.exit_code, + output_directory: self.output_directory, + execution_attempts, + } + } +} + /// ???/just a neat way to separate logic as this interface evolves, not really necessary right now pub trait SerializableProcessExecutionCodec< // TODO: add some constraints to these? @@ -438,6 +480,10 @@ pub trait ImmediateExecutionCache: Send + Sync { #[derive(Clone)] pub struct ActionCache { store: fs::Store, + // NB: This could be an Arc>> if we ever need to + // add any further such codecs. This type is static in this struct, because the codec is required + // to produce specifically Action and ActionResult, because that is what is currently accepted by + // `store.record_process_result()`. process_execution_codec: BazelProtosProcessExecutionCodec, } @@ -461,11 +507,19 @@ impl ImmediateExecutionCache>, + cache: Arc< + Box>, + >, + cache_key_gen_version: Option, +} + +impl CachingCommandRunner { + pub fn from_store( + inner: Box, + store: fs::Store, + cache_key_gen_version: Option, + ) -> Self { + let action_cache = ActionCache::new(store); + let boxed_cache = Box::new(action_cache) + as Box< + dyn ImmediateExecutionCache, + >; + Self::new(inner, boxed_cache, cache_key_gen_version) + } + + pub fn new( + inner: Box, + cache: Box< + dyn ImmediateExecutionCache, + >, + cache_key_gen_version: Option, + ) -> Self { + CachingCommandRunner { + inner: Arc::new(inner), + cache: Arc::new(cache), + cache_key_gen_version, + } + } +} + +impl CommandRunner for CachingCommandRunner { + fn run(&self, req: ExecuteProcessRequest) -> BoxFuture { + let cacheable_request = + CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); + let cache = self.cache.clone(); + let inner = self.inner.clone(); + cache + .load_process_result(cacheable_request.clone()) + .and_then(move |cache_fetch| match cache_fetch { + // We have a cache hit! + Some(cached_execution_result) => { + debug!( + "cached execution for request {:?}! {:?}", + req.clone(), + cached_execution_result + ); + future::result(Ok(cached_execution_result)).to_boxed() + } + // We have to actually run the process now. + None => inner + .run(req.clone()) + .and_then(move |res| { + debug!("uncached execution for request {:?}: {:?}", req, res); + let cacheable_process_result = res.without_execution_attempts(); + cache + .record_process_result(cacheable_request.clone(), cacheable_process_result.clone()) + .map(move |()| { + debug!( + "request {:?} should now be cached as {:?}", + cacheable_request.clone(), + cacheable_process_result.clone() + ); + cacheable_process_result + }) + }) + .to_boxed(), + }) + // NB: We clear metadata about execution attempts when returning a cacheable process execution + // result. + .map(|cacheable_process_result| cacheable_process_result.with_execution_attempts(vec![])) + .to_boxed() + } +} diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 2be7d4cad9a..29e5aa0e0c1 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -30,9 +30,8 @@ // Arc can be more clear than needing to grok Orderings: #![allow(clippy::mutex_atomic)] -use boxfuture::{BoxFuture, Boxable}; +use boxfuture::BoxFuture; use bytes::Bytes; -use futures::future::{self, Future}; use std::collections::{BTreeMap, BTreeSet}; use std::ops::AddAssign; use std::path::PathBuf; @@ -45,7 +44,8 @@ pub mod cached_execution; pub mod local; pub mod remote; pub use crate::cached_execution::{ - ActionCache, BazelProtosProcessExecutionCodec, ImmediateExecutionCache, + ActionCache, BazelProtosProcessExecutionCodec, CacheableExecuteProcessRequest, + CacheableExecuteProcessResult, CachingCommandRunner, ImmediateExecutionCache, SerializableProcessExecutionCodec, }; @@ -92,23 +92,6 @@ pub struct ExecuteProcessRequest { pub jdk_home: Option, } -/// ???/DON'T LET THE `cache_key_gen_version` BECOME A KITCHEN SINK!!! -#[derive(Clone)] -pub struct CacheableExecuteProcessRequest { - req: ExecuteProcessRequest, - // TODO: give this a better type than Option (everywhere)! - cache_key_gen_version: Option, -} - -impl CacheableExecuteProcessRequest { - fn new(req: ExecuteProcessRequest, cache_key_gen_version: Option) -> Self { - CacheableExecuteProcessRequest { - req, - cache_key_gen_version, - } - } -} - /// /// The result of running a process. /// @@ -126,7 +109,7 @@ pub struct FallibleExecuteProcessResult { } impl FallibleExecuteProcessResult { - fn without_execution_attempts(&self) -> CacheableExecuteProcessResult { + pub fn without_execution_attempts(&self) -> CacheableExecuteProcessResult { CacheableExecuteProcessResult { stdout: self.stdout.clone(), stderr: self.stderr.clone(), @@ -136,29 +119,6 @@ impl FallibleExecuteProcessResult { } } -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct CacheableExecuteProcessResult { - pub stdout: Bytes, - pub stderr: Bytes, - pub exit_code: i32, - pub output_directory: hashing::Digest, -} - -impl CacheableExecuteProcessResult { - fn with_execution_attempts( - &self, - execution_attempts: Vec, - ) -> FallibleExecuteProcessResult { - FallibleExecuteProcessResult { - stdout: self.stdout.clone(), - stderr: self.stderr.clone(), - exit_code: self.exit_code, - output_directory: self.output_directory, - execution_attempts, - } - } -} - // TODO: remove this method! #[cfg(test)] impl FallibleExecuteProcessResult { @@ -214,73 +174,3 @@ impl CommandRunner for BoundedCommandRunner { self.inner.1.with_acquired(move || inner.0.run(req)) } } - -/// -/// A CommandRunner wrapper that attempts to cache process executions. -/// -#[derive(Clone)] -pub struct CachingCommandRunner { - inner: Arc>, - cache: Arc< - Box>, - >, - cache_key_gen_version: Option, -} - -impl CachingCommandRunner { - pub fn from_store( - inner: Box, - store: fs::Store, - cache_key_gen_version: Option, - ) -> Self { - let action_cache = ActionCache::new(store); - let boxed_cache = Box::new(action_cache) - as Box< - dyn ImmediateExecutionCache, - >; - Self::new(inner, boxed_cache, cache_key_gen_version) - } - - pub fn new( - inner: Box, - cache: Box< - dyn ImmediateExecutionCache, - >, - cache_key_gen_version: Option, - ) -> Self { - CachingCommandRunner { - inner: Arc::new(inner), - cache: Arc::new(cache), - cache_key_gen_version, - } - } -} - -impl CommandRunner for CachingCommandRunner { - fn run(&self, req: ExecuteProcessRequest) -> BoxFuture { - let cacheable_request = - CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); - let cache = self.cache.clone(); - let inner = self.inner.clone(); - cache - .load_process_result(cacheable_request.clone()) - .and_then(move |cache_fetch| match cache_fetch { - // We have a cache hit! - Some(cached_execution_result) => future::result(Ok(cached_execution_result)).to_boxed(), - // We have to actually run the process now. - None => inner - .run(req) - .and_then(move |res| { - let cacheable_process_result = res.without_execution_attempts(); - cache - .record_process_result(cacheable_request, cacheable_process_result.clone()) - .map(|()| cacheable_process_result) - }) - .to_boxed(), - }) - // NB: We clear metadata about execution attempts when returning a cacheable process execution - // request. - .map(|cacheable_process_result| cacheable_process_result.with_execution_attempts(vec![])) - .to_boxed() - } -} diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index aa589799915..493feb228c7 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -171,6 +171,73 @@ impl BazelProtosProcessExecutionCodecV2 { resulting_output_dir } + fn convert_timestamp(timestamp: prost_types::Timestamp) -> protobuf::well_known_types::Timestamp { + let mut resulting_timestamp = protobuf::well_known_types::Timestamp::new(); + resulting_timestamp.set_seconds(timestamp.seconds); + resulting_timestamp.set_nanos(timestamp.nanos); + resulting_timestamp + } + + fn convert_execution_metadata( + exec_metadata: bazel_protos::build::bazel::remote::execution::v2::ExecutedActionMetadata, + ) -> bazel_protos::remote_execution::ExecutedActionMetadata { + let mut resulting_exec_metadata = bazel_protos::remote_execution::ExecutedActionMetadata::new(); + resulting_exec_metadata.set_worker(exec_metadata.worker); + if let Some(queued_timestamp) = exec_metadata.queued_timestamp.map(Self::convert_timestamp) { + resulting_exec_metadata.set_queued_timestamp(queued_timestamp); + } + if let Some(worker_start_timestamp) = exec_metadata + .worker_start_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_worker_start_timestamp(worker_start_timestamp); + } + if let Some(worker_completed_timestamp) = exec_metadata + .worker_completed_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_worker_completed_timestamp(worker_completed_timestamp); + } + if let Some(input_fetch_start_timestamp) = exec_metadata + .input_fetch_start_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_input_fetch_start_timestamp(input_fetch_start_timestamp); + } + if let Some(input_fetch_completed_timestamp) = exec_metadata + .input_fetch_completed_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_input_fetch_completed_timestamp(input_fetch_completed_timestamp); + } + if let Some(execution_start_timestamp) = exec_metadata + .execution_start_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_execution_start_timestamp(execution_start_timestamp); + } + if let Some(execution_completed_timestamp) = exec_metadata + .execution_completed_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_execution_completed_timestamp(execution_completed_timestamp); + } + if let Some(output_upload_start_timestamp) = exec_metadata + .output_upload_start_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata.set_output_upload_start_timestamp(output_upload_start_timestamp); + } + if let Some(output_upload_completed_timestamp) = exec_metadata + .output_upload_completed_timestamp + .map(Self::convert_timestamp) + { + resulting_exec_metadata + .set_output_upload_completed_timestamp(output_upload_completed_timestamp); + } + resulting_exec_metadata + } + fn convert_action_result( action_result: bazel_protos::build::bazel::remote::execution::v2::ActionResult, ) -> bazel_protos::remote_execution::ActionResult { @@ -181,7 +248,7 @@ impl BazelProtosProcessExecutionCodecV2 { .iter() .cloned() .map(Self::convert_output_file) - .collect::>(), + .collect(), )); resulting_action_result.set_output_directories(protobuf::RepeatedField::from_vec( action_result @@ -200,7 +267,12 @@ impl BazelProtosProcessExecutionCodecV2 { if let Some(digest) = action_result.stderr_digest.map(Self::convert_digest) { resulting_action_result.set_stderr_digest(digest); } - // TODO: resulting_action_result.set_execution_metadata(); + if let Some(execution_metadata) = action_result + .execution_metadata + .map(Self::convert_execution_metadata) + { + resulting_action_result.set_execution_metadata(execution_metadata); + } resulting_action_result } } From 435718243e3c9e3c2ef534271d20ef4728a9d8e1 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 02:46:50 -0800 Subject: [PATCH 03/10] add testing and make other tests compile and pass! --- src/rust/engine/fs/src/store.rs | 2 - .../process_execution/src/cached_execution.rs | 305 ++++++++++++++---- src/rust/engine/process_execution/src/lib.rs | 6 +- .../engine/process_execution/src/local.rs | 1 + .../engine/process_execution/src/remote.rs | 133 ++++++-- 5 files changed, 353 insertions(+), 94 deletions(-) diff --git a/src/rust/engine/fs/src/store.rs b/src/rust/engine/fs/src/store.rs index faa5a7539b1..8bf12cd19d0 100644 --- a/src/rust/engine/fs/src/store.rs +++ b/src/rust/engine/fs/src/store.rs @@ -2572,14 +2572,12 @@ mod tests { use crate::pool::ResettablePool; use bazel_protos; use bytes::Bytes; - use digest::{Digest as DigestTrait, FixedOutput}; use futures::Future; use futures_timer::TimerHandle; use hashing::{Digest, Fingerprint}; use mock::StubCAS; use protobuf::Message; use serverset::BackoffConfig; - use sha2::Sha256; use std; use std::collections::HashMap; use std::fs::File; diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index 369e3e5c67c..75ef5909d94 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -48,7 +48,7 @@ use super::{CommandRunner, ExecuteProcessRequest, ExecutionStats, FallibleExecut // Environment variable which is exclusively used for cache key invalidation. // This may be not specified in an ExecuteProcessRequest, and may be populated only by the // CommandRunner. -const CACHE_KEY_GEN_VERSION_ENV_VAR_NAME: &str = "PANTS_CACHE_KEY_GEN_VERSION"; +pub const CACHE_KEY_GEN_VERSION_ENV_VAR_NAME: &str = "PANTS_CACHE_KEY_GEN_VERSION"; /// ???/DON'T LET THE `cache_key_gen_version` BECOME A KITCHEN SINK!!! #[derive(Clone, Debug, Eq, PartialEq)] @@ -120,6 +120,61 @@ pub struct BazelProtosProcessExecutionCodec { store: fs::Store, } +impl + SerializableProcessExecutionCodec< + CacheableExecuteProcessRequest, + bazel_protos::remote_execution::Action, + CacheableExecuteProcessResult, + bazel_protos::remote_execution::ActionResult, + // TODO: better error type? + String, + > for BazelProtosProcessExecutionCodec +{ + fn convert_request( + &self, + req: CacheableExecuteProcessRequest, + ) -> Result { + let (action, _) = Self::make_action_with_command(req)?; + Ok(action) + } + + fn convert_response( + &self, + res: CacheableExecuteProcessResult, + ) -> Result { + let mut action_proto = bazel_protos::remote_execution::ActionResult::new(); + let mut output_directory = bazel_protos::remote_execution::OutputDirectory::new(); + let output_directory_digest = Self::convert_digest(res.output_directory); + output_directory.set_tree_digest(output_directory_digest); + action_proto.set_output_directories(protobuf::RepeatedField::from_vec(vec![output_directory])); + action_proto.set_exit_code(res.exit_code); + action_proto.set_stdout_raw(res.stdout.clone()); + action_proto.set_stdout_digest(Self::convert_digest(Self::digest_bytes(&res.stdout))); + action_proto.set_stderr_raw(res.stderr.clone()); + action_proto.set_stderr_digest(Self::convert_digest(Self::digest_bytes(&res.stderr))); + Ok(action_proto) + } + + fn extract_response( + &self, + res: bazel_protos::remote_execution::ActionResult, + ) -> BoxFuture { + self + .extract_stdout(res.clone()) + .join(self.extract_stderr(res.clone())) + .join(self.extract_output_files(res.clone())) + .map( + move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { + stdout, + stderr, + exit_code: res.exit_code, + output_directory, + }, + ) + .to_boxed() + } +} + impl BazelProtosProcessExecutionCodec { pub fn new(store: fs::Store) -> Self { BazelProtosProcessExecutionCodec { store } @@ -413,61 +468,6 @@ impl BazelProtosProcessExecutionCodec { } } -impl - SerializableProcessExecutionCodec< - CacheableExecuteProcessRequest, - bazel_protos::remote_execution::Action, - CacheableExecuteProcessResult, - bazel_protos::remote_execution::ActionResult, - // TODO: better error type? - String, - > for BazelProtosProcessExecutionCodec -{ - fn convert_request( - &self, - req: CacheableExecuteProcessRequest, - ) -> Result { - let (action, _) = Self::make_action_with_command(req)?; - Ok(action) - } - - fn convert_response( - &self, - res: CacheableExecuteProcessResult, - ) -> Result { - let mut action_proto = bazel_protos::remote_execution::ActionResult::new(); - let mut output_directory = bazel_protos::remote_execution::OutputDirectory::new(); - let output_directory_digest = Self::convert_digest(res.output_directory); - output_directory.set_tree_digest(output_directory_digest); - action_proto.set_output_directories(protobuf::RepeatedField::from_vec(vec![output_directory])); - action_proto.set_exit_code(res.exit_code); - action_proto.set_stdout_raw(res.stdout.clone()); - action_proto.set_stdout_digest(Self::convert_digest(Self::digest_bytes(&res.stdout))); - action_proto.set_stderr_raw(res.stderr.clone()); - action_proto.set_stderr_digest(Self::convert_digest(Self::digest_bytes(&res.stderr))); - Ok(action_proto) - } - - fn extract_response( - &self, - res: bazel_protos::remote_execution::ActionResult, - ) -> BoxFuture { - self - .extract_stdout(res.clone()) - .join(self.extract_stderr(res.clone())) - .join(self.extract_output_files(res.clone())) - .map( - move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { - stdout, - stderr, - exit_code: res.exit_code, - output_directory, - }, - ) - .to_boxed() - } -} - /// ???/it's called "immediate" because it's a best-effort thing located locally (???) pub trait ImmediateExecutionCache: Send + Sync { fn record_process_result(&self, req: ProcessRequest, res: ProcessResult) @@ -516,7 +516,7 @@ impl ImmediateExecutionCache() + .unwrap(); + // Try again and verify the result is cached (the random number is still the same). + let second_process_result = caching_runner.run(random_perl.clone()).wait().unwrap(); + let second_perl_number = String::from_utf8(second_process_result.stdout.deref().to_vec()) + .unwrap() + .parse::() + .unwrap(); + assert_eq!(perl_number, second_perl_number); + // See that the result is invalidated if a `cache_key_gen_version` is provided. + let new_key = "xx".to_string(); + let new_cacheable_perl = CacheableExecuteProcessRequest { + req: random_perl.clone(), + cache_key_gen_version: Some(new_key.clone()), + }; + assert_eq!( + Ok(None), + action_cache + .load_process_result(new_cacheable_perl.clone()) + .wait() + ); + let new_caching_runner = + make_caching_runner(base_runner.clone(), action_cache.clone(), Some(new_key)); + let new_process_result = new_caching_runner.run(random_perl.clone()).wait().unwrap(); + assert_eq!(0, new_process_result.exit_code); + // The new `cache_key_gen_version` is propagated to the requests made against the + // CachingCommandRunner. + assert_eq!( + new_process_result.clone().into_cacheable(), + action_cache + .load_process_result(new_cacheable_perl) + .wait() + .unwrap() + .unwrap() + ); + let new_perl_number = String::from_utf8(new_process_result.stdout.deref().to_vec()) + .unwrap() + .parse::() + .unwrap(); + // The output of the rand(10) call in the perl invocation is different, because the process + // execution wasn't cached. + assert!(new_perl_number != perl_number); + // Make sure that changing the cache key string from non-None to non-None also invalidates the + // process result. + let second_string_key = "yy".to_string(); + let second_cache_string_perl = CacheableExecuteProcessRequest { + req: random_perl.clone(), + cache_key_gen_version: Some(second_string_key.clone()), + }; + assert_eq!( + Ok(None), + action_cache + .load_process_result(second_cache_string_perl.clone()) + .wait() + ); + let second_string_caching_runner = make_caching_runner( + base_runner.clone(), + action_cache.clone(), + Some(second_string_key), + ); + let second_string_process_result = second_string_caching_runner + .run(random_perl) + .wait() + .unwrap(); + assert_eq!(0, second_string_process_result.exit_code); + assert_eq!( + second_string_process_result.clone().into_cacheable(), + action_cache + .load_process_result(second_cache_string_perl) + .wait() + .unwrap() + .unwrap() + ); + let second_string_perl_number = + String::from_utf8(second_string_process_result.stdout.deref().to_vec()) + .unwrap() + .parse::() + .unwrap(); + // The new result is distinct from all the previously cached invocations. + assert!(second_string_perl_number != perl_number); + assert!(second_string_perl_number != new_perl_number); + } + + fn output_only_process_request(argv: Vec) -> ExecuteProcessRequest { + ExecuteProcessRequest { + argv, + env: BTreeMap::new(), + input_files: fs::EMPTY_DIGEST, + output_files: BTreeSet::new(), + output_directories: BTreeSet::new(), + timeout: Duration::from_millis(1000), + description: "write some output".to_string(), + jdk_home: None, + } + } + + fn cache_in_dir( + store_dir: &Path, + work_dir: &Path, + ) -> (fs::Store, crate::local::CommandRunner, ActionCache) { + let pool = Arc::new(fs::ResettablePool::new("test-pool-".to_owned())); + let store = fs::Store::local_only(store_dir, pool.clone()).unwrap(); + let action_cache = ActionCache::new(store.clone()); + let base_runner = + crate::local::CommandRunner::new(store.clone(), pool, work_dir.to_path_buf(), true); + (store, base_runner, action_cache) + } + + fn make_caching_runner( + base_runner: crate::local::CommandRunner, + action_cache: ActionCache, + cache_key_gen_version: Option, + ) -> CachingCommandRunner { + CachingCommandRunner::new( + Box::new(base_runner) as Box, + Box::new(action_cache) + as Box< + dyn ImmediateExecutionCache< + CacheableExecuteProcessRequest, + CacheableExecuteProcessResult, + >, + >, + cache_key_gen_version, + ) + } +} diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 29e5aa0e0c1..32aa4746ea5 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -109,10 +109,10 @@ pub struct FallibleExecuteProcessResult { } impl FallibleExecuteProcessResult { - pub fn without_execution_attempts(&self) -> CacheableExecuteProcessResult { + pub fn into_cacheable(self) -> CacheableExecuteProcessResult { CacheableExecuteProcessResult { - stdout: self.stdout.clone(), - stderr: self.stderr.clone(), + stdout: self.stdout, + stderr: self.stderr, exit_code: self.exit_code, output_directory: self.output_directory, } diff --git a/src/rust/engine/process_execution/src/local.rs b/src/rust/engine/process_execution/src/local.rs index baf052442cd..5e24d85aecd 100644 --- a/src/rust/engine/process_execution/src/local.rs +++ b/src/rust/engine/process_execution/src/local.rs @@ -21,6 +21,7 @@ use super::{ExecuteProcessRequest, FallibleExecuteProcessResult}; use bytes::{Bytes, BytesMut}; +#[derive(Clone)] pub struct CommandRunner { store: fs::Store, fs_pool: Arc, diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 493feb228c7..7011b2e6684 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -882,9 +882,11 @@ mod tests { use super::super::CommandRunner as CommandRunnerTrait; use super::{ - CommandRunner, ExecuteProcessRequest, ExecutionError, ExecutionHistory, - FallibleExecuteProcessResult, + BazelProcessExecutionRequestV2, BazelProtosProcessExecutionCodec, + BazelProtosProcessExecutionCodecV2, CacheableExecuteProcessRequest, CommandRunner, + ExecuteProcessRequest, ExecutionError, ExecutionHistory, FallibleExecuteProcessResult, }; + use crate::cached_execution::SerializableProcessExecutionCodec; use mock::execution_server::MockOperation; use std::collections::{BTreeMap, BTreeSet}; use std::iter::{self, FromIterator}; @@ -906,7 +908,7 @@ mod tests { } #[test] - fn make_execute_request() { + fn test_make_execute_request() { let input_directory = TestDirectory::containing_roland(); let req = ExecuteProcessRequest { argv: owned_string_vec(&["/bin/echo", "yo"]), @@ -975,7 +977,7 @@ mod tests { }; assert_eq!( - super::make_execute_request(&req, &None, &None), + make_execute_request(&req, &None, &None), Ok((want_action, want_command, want_execute_request)) ); } @@ -1064,7 +1066,7 @@ mod tests { }; assert_eq!( - super::make_execute_request(&req, &Some("dark-tower".to_owned()), &None), + make_execute_request(&req, &Some("dark-tower".to_owned()), &None), Ok((want_action, want_command, want_execute_request)) ); } @@ -1103,7 +1105,7 @@ mod tests { }); want_command.mut_environment_variables().push({ let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); - env.set_name(super::CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_owned()); + env.set_name(crate::cached_execution::CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_owned()); env.set_value("meep".to_owned()); env }); @@ -1145,7 +1147,7 @@ mod tests { }; assert_eq!( - super::make_execute_request(&req, &None, &Some("meep".to_owned())), + make_execute_request(&req, &None, &Some("meep".to_owned())), Ok((want_action, want_command, want_execute_request)) ); } @@ -1202,7 +1204,7 @@ mod tests { }; assert_eq!( - super::make_execute_request(&req, &None, &None), + make_execute_request(&req, &None, &None), Ok((want_action, want_command, want_execute_request)) ); } @@ -1214,7 +1216,7 @@ mod tests { let mock_server = { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( "wrong-command".to_string(), - super::make_execute_request( + make_execute_request( &ExecuteProcessRequest { argv: owned_string_vec(&["/bin/echo", "-n", "bar"]), env: BTreeMap::new(), @@ -1250,7 +1252,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1347,7 +1349,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&echo_roland_request(), &None, &None) + make_execute_request(&echo_roland_request(), &None, &None) .unwrap() .2, vec![make_successful_operation( @@ -1436,7 +1438,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, Vec::from_iter( @@ -1487,7 +1489,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1512,7 +1514,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1551,7 +1553,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1585,7 +1587,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![MockOperation::new( @@ -1619,7 +1621,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1654,7 +1656,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![MockOperation::new( @@ -1682,7 +1684,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -1711,7 +1713,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&cat_roland_request(), &None, &None) + make_execute_request(&cat_roland_request(), &None, &None) .unwrap() .2, vec![ @@ -1804,7 +1806,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&cat_roland_request(), &None, &None) + make_execute_request(&cat_roland_request(), &None, &None) .unwrap() .2, vec![ @@ -1886,7 +1888,7 @@ mod tests { mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&cat_roland_request(), &None, &None) + make_execute_request(&cat_roland_request(), &None, &None) .unwrap() .2, // We won't get as far as trying to run the operation, so don't expect any requests whose @@ -2145,7 +2147,7 @@ mod tests { env2.set_value("b".to_string()); command.mut_environment_variables().push(env2); - let digest = super::digest(&command).unwrap(); + let digest = BazelProtosProcessExecutionCodec::digest_message(&command).unwrap(); assert_eq!( &digest.0.to_hex(), @@ -2163,7 +2165,7 @@ mod tests { let op_name = "gimme-foo".to_string(); mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -2201,7 +2203,7 @@ mod tests { let op_name = "gimme-foo".to_string(); mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new( op_name.clone(), - super::make_execute_request(&execute_request, &None, &None) + make_execute_request(&execute_request, &None, &None) .unwrap() .2, vec![ @@ -2508,14 +2510,18 @@ mod tests { result } - fn create_command_runner(address: String, cas: &mock::StubCAS) -> CommandRunner { + fn create_command_runner_with_store( + address: String, + cas: &mock::StubCAS, + instance_name: Option, + ) -> (fs::Store, CommandRunner) { let store_dir = TempDir::new().unwrap(); let timer_thread = timer_thread(); let store = fs::Store::with_remote( store_dir, Arc::new(fs::ResettablePool::new("test-pool-".to_owned())), &[cas.address()], - None, + instance_name, &None, None, 1, @@ -2527,8 +2533,15 @@ mod tests { ) .expect("Failed to make store"); - CommandRunner::new(&address, None, None, None, store, timer_thread) - .expect("Failed to make command runner") + let command_runner = + CommandRunner::new(&address, None, None, None, store.clone(), timer_thread) + .expect("Failed to make command runner"); + + (store, command_runner) + } + + fn create_command_runner(address: String, cas: &mock::StubCAS) -> CommandRunner { + create_command_runner_with_store(address, cas, None).1 } fn timer_thread() -> resettable::Resettable { @@ -2561,11 +2574,15 @@ mod tests { .directory(&TestDirectory::containing_roland()) .build(); + let (store, _) = create_command_runner_with_store("127.0.0.1:0".to_owned(), &cas, None); + let codec = codec_from_store(store, None); + let mut runtime = tokio::runtime::Runtime::new().unwrap(); - let command_runner = create_command_runner("127.0.0.1:0".to_owned(), &cas); - let result = runtime.block_on(command_runner.extract_output_files(result)); + let result = runtime.block_on(codec.extract_response(result.clone())); runtime.shutdown_now().wait().unwrap(); result + .map(|res| res.output_directory) + .map_err(|e| ExecutionError::Fatal(e)) } fn make_any_prost_executeresponse( @@ -2630,4 +2647,58 @@ mod tests { jdk_home: None, } } + + fn codec_from_store( + store: fs::Store, + instance_name: Option, + ) -> BazelProtosProcessExecutionCodecV2 { + BazelProtosProcessExecutionCodecV2::new(store, instance_name) + } + + fn codec(dir: PathBuf, instance_name: Option) -> BazelProtosProcessExecutionCodecV2 { + let pool = Arc::new(fs::ResettablePool::new("test-pool-".to_owned())); + let store = fs::Store::local_only(dir, pool.clone()).unwrap(); + codec_from_store(store, instance_name) + } + + fn codec_convert_execute_request( + dir: PathBuf, + req: ExecuteProcessRequest, + instance_name: Option, + cache_key_gen_version: Option, + ) -> Result { + let codec = codec(dir, instance_name); + codec.convert_request(CacheableExecuteProcessRequest::new( + req, + cache_key_gen_version, + )) + } + + fn make_execute_request( + req: &ExecuteProcessRequest, + instance_name: &Option, + cache_key_gen_version: &Option, + ) -> Result< + ( + bazel_protos::remote_execution::Action, + bazel_protos::remote_execution::Command, + bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, + ), + String, + > { + let store_dir = TempDir::new().unwrap(); + codec_convert_execute_request( + store_dir.path().to_path_buf(), + req.clone(), + instance_name.clone(), + cache_key_gen_version.clone(), + ) + .map( + |BazelProcessExecutionRequestV2 { + action, + command, + execute_request, + }| { (action, command, execute_request) }, + ) + } } From 4c34f828819e57059660ae9b54047b32f5e0036a Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 15:57:41 -0800 Subject: [PATCH 04/10] convert old protos to new tower protos --- src/rust/engine/Cargo.lock | 1 + src/rust/engine/fs/Cargo.toml | 1 + src/rust/engine/fs/src/store.rs | 97 +++-- .../process_execution/src/cached_execution.rs | 396 ++++++++---------- src/rust/engine/process_execution/src/lib.rs | 5 +- .../engine/process_execution/src/remote.rs | 298 +++---------- 6 files changed, 316 insertions(+), 482 deletions(-) diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 4c13841e6d2..a3c79591aaa 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -529,6 +529,7 @@ dependencies = [ "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "mock 0.0.1", "parking_lot 0.6.4 (registry+https://github.com/rust-lang/crates.io-index)", + "prost 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "protobuf 2.0.5 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.80 (registry+https://github.com/rust-lang/crates.io-index)", "serde_derive 1.0.58 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/src/rust/engine/fs/Cargo.toml b/src/rust/engine/fs/Cargo.toml index d94f98d38cd..e5fda7143f6 100644 --- a/src/rust/engine/fs/Cargo.toml +++ b/src/rust/engine/fs/Cargo.toml @@ -25,6 +25,7 @@ lazy_static = "1" lmdb = { git = "https://github.com/pantsbuild/lmdb-rs.git", rev = "06bdfbfc6348f6804127176e561843f214fc17f8" } log = "0.4" parking_lot = "0.6" +prost = "0.4" protobuf = { version = "2.0.4", features = ["with-bytes"] } serverset = { path = "../serverset" } sha2 = "0.8" diff --git a/src/rust/engine/fs/src/store.rs b/src/rust/engine/fs/src/store.rs index 8bf12cd19d0..898b6c995cd 100644 --- a/src/rust/engine/fs/src/store.rs +++ b/src/rust/engine/fs/src/store.rs @@ -2,10 +2,11 @@ use crate::{BackoffConfig, FileContent}; use bazel_protos; use boxfuture::{try_future, BoxFuture, Boxable}; -use bytes::Bytes; +use bytes::{Bytes, BytesMut}; use dirs; use futures::{future, Future}; use hashing::{Digest, Fingerprint}; +use prost::{self, Message as ProstMessage}; use protobuf::Message; use serde_derive::Serialize; use std::collections::HashMap; @@ -82,13 +83,6 @@ pub enum ShrinkBehavior { // This has the nice property that Directories can be trusted to be valid and canonical. // We may want to re-visit this if we end up wanting to handle local/remote/merged interchangably. impl Store { - pub fn fingerprint_from_bytes_unsafe(bytes: &Bytes) -> Fingerprint { - use digest::{Digest as DigestTrait, FixedOutput}; - let mut hasher = sha2::Sha256::default(); - hasher.input(&bytes); - Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()) - } - /// /// Make a store which only uses its local storage. /// @@ -240,28 +234,60 @@ impl Store { ) } + pub fn fingerprint_from_bytes_unsafe(bytes: &Bytes) -> Fingerprint { + use digest::{Digest as DigestTrait, FixedOutput}; + let mut hasher = sha2::Sha256::default(); + hasher.input(&bytes); + Fingerprint::from_bytes_unsafe(hasher.fixed_result().as_slice()) + } + + pub fn digest_bytes(bytes: &Bytes) -> Digest { + let fingerprint = Self::fingerprint_from_bytes_unsafe(&bytes); + Digest(fingerprint, bytes.len()) + } + + // TODO: is there an existing way to do this conversion (without explicitly allocating a buf of + // the same length)? + pub fn encode_proto(proto: &P) -> Result { + let mut buf = BytesMut::with_capacity(proto.encoded_len()); + proto.encode(&mut buf)?; + Ok(buf.freeze()) + } + + pub fn encode_action_proto( + action: &bazel_protos::build::bazel::remote::execution::v2::Action, + ) -> Result { + Self::encode_proto(action) + .map_err(|e| format!("Error serializing Action proto {:?}: {:?}", action, e)) + } + + pub fn encode_action_result_proto( + action_result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, + ) -> Result { + Self::encode_proto(action_result).map_err(|e| { + format!( + "Error serializing ActionResult proto {:?}: {:?}", + action_result, e + ) + }) + } + // TODO: convert every Result<_, String> into a typedef! fn action_fingerprint( - action: bazel_protos::remote_execution::Action, + action: &bazel_protos::build::bazel::remote::execution::v2::Action, ) -> Result { - action - .write_to_bytes() - .map_err(|e| format!("Error serializing Action proto {:?}: {:?}", action, e)) - .map(|bytes| super::Store::fingerprint_from_bytes_unsafe(&Bytes::from(bytes))) + Self::encode_action_proto(&action) + .map(|bytes| super::Store::fingerprint_from_bytes_unsafe(&bytes)) } /// ??? pub fn record_process_result( &self, - req: bazel_protos::remote_execution::Action, - res: bazel_protos::remote_execution::ActionResult, + req: &bazel_protos::build::bazel::remote::execution::v2::Action, + res: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, ) -> BoxFuture<(), String> { - let converted_key_value_protos = Self::action_fingerprint(req).and_then(|req| { - res - .write_to_bytes() - .map_err(|e| format!("Error serializing ActionResult proto {:?}: {:?}", res, e)) - .map(|res| (req, Bytes::from(res))) - }); + let converted_key_value_protos = Self::action_fingerprint(&req) + .and_then(|req| Self::encode_action_result_proto(&res).map(|res| (req, res))); let store = self.clone(); future::result(converted_key_value_protos) .and_then(move |(action_fingerprint, result_bytes)| { @@ -273,29 +299,30 @@ impl Store { } fn deserialize_action_result( - result_bytes: Bytes, - ) -> Result { - let mut action_result = bazel_protos::remote_execution::ActionResult::new(); - action_result.merge_from_bytes(&result_bytes).map_err(|e| { - format!( - "LMDB corruption: ActionResult bytes for {:?} were not valid: {:?}", - result_bytes, e - ) - })?; - Ok(action_result) + result_bytes: &Bytes, + ) -> Result { + bazel_protos::build::bazel::remote::execution::v2::ActionResult::decode(result_bytes).map_err( + |e| { + format!( + "LMDB corruption: ActionResult bytes for {:?} were not valid: {:?}", + result_bytes, e + ) + }, + ) } /// ??? pub fn load_process_result( &self, - req: bazel_protos::remote_execution::Action, - ) -> BoxFuture, String> { + req: &bazel_protos::build::bazel::remote::execution::v2::Action, + ) -> BoxFuture, String> + { let store = self.clone(); - future::result(Self::action_fingerprint(req)) + future::result(Self::action_fingerprint(&req)) .and_then(move |action_fingerprint| store.local.load_process_result(action_fingerprint)) .and_then(|maybe_bytes| { let deserialized_result = match maybe_bytes { - Some(bytes) => Self::deserialize_action_result(bytes).map(Some), + Some(bytes) => Self::deserialize_action_result(&bytes).map(Some), None => Ok(None), }; future::result(deserialized_result) diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index 75ef5909d94..832fe4aeeb5 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -38,7 +38,6 @@ use boxfuture::{try_future, BoxFuture, Boxable}; use bytes::Bytes; use futures::future::{self, Future}; use log::debug; -use protobuf::{self, Message as GrpcioMessage}; use fs::{File, PathStat}; use hashing::Digest; @@ -67,6 +66,7 @@ impl CacheableExecuteProcessRequest { } } +/// ???/why is this "cacheable"? #[derive(Clone, Debug, Eq, PartialEq)] pub struct CacheableExecuteProcessResult { pub stdout: Bytes, @@ -90,142 +90,70 @@ impl CacheableExecuteProcessResult { } } -/// ???/just a neat way to separate logic as this interface evolves, not really necessary right now -pub trait SerializableProcessExecutionCodec< - // TODO: add some constraints to these? - ProcessRequest, - SerializableRequest, - ProcessResult, - SerializableResult, - ErrorType, - >: Send + Sync { - fn convert_request( - &self, - req: ProcessRequest - ) -> Result; - - fn convert_response( +/// ???/it's called "immediate" because it's a best-effort thing located locally (???) +pub trait ImmediateExecutionCache: Send + Sync { + fn record_process_result( &self, - res: ProcessResult, - ) -> Result; + req: &ProcessRequest, + res: &ProcessResult, + ) -> BoxFuture<(), String>; - fn extract_response( - &self, - serializable_response: SerializableResult, - ) -> BoxFuture; + fn load_process_result(&self, req: &ProcessRequest) -> BoxFuture, String>; } +/// ??? #[derive(Clone)] -pub struct BazelProtosProcessExecutionCodec { +pub struct ActionSerializer { store: fs::Store, } -impl - SerializableProcessExecutionCodec< - CacheableExecuteProcessRequest, - bazel_protos::remote_execution::Action, - CacheableExecuteProcessResult, - bazel_protos::remote_execution::ActionResult, - // TODO: better error type? - String, - > for BazelProtosProcessExecutionCodec -{ - fn convert_request( - &self, - req: CacheableExecuteProcessRequest, - ) -> Result { - let (action, _) = Self::make_action_with_command(req)?; - Ok(action) - } - - fn convert_response( - &self, - res: CacheableExecuteProcessResult, - ) -> Result { - let mut action_proto = bazel_protos::remote_execution::ActionResult::new(); - let mut output_directory = bazel_protos::remote_execution::OutputDirectory::new(); - let output_directory_digest = Self::convert_digest(res.output_directory); - output_directory.set_tree_digest(output_directory_digest); - action_proto.set_output_directories(protobuf::RepeatedField::from_vec(vec![output_directory])); - action_proto.set_exit_code(res.exit_code); - action_proto.set_stdout_raw(res.stdout.clone()); - action_proto.set_stdout_digest(Self::convert_digest(Self::digest_bytes(&res.stdout))); - action_proto.set_stderr_raw(res.stderr.clone()); - action_proto.set_stderr_digest(Self::convert_digest(Self::digest_bytes(&res.stderr))); - Ok(action_proto) - } - - fn extract_response( - &self, - res: bazel_protos::remote_execution::ActionResult, - ) -> BoxFuture { - self - .extract_stdout(res.clone()) - .join(self.extract_stderr(res.clone())) - .join(self.extract_output_files(res.clone())) - .map( - move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { - stdout, - stderr, - exit_code: res.exit_code, - output_directory, - }, - ) - .to_boxed() - } -} - -impl BazelProtosProcessExecutionCodec { +impl ActionSerializer { pub fn new(store: fs::Store) -> Self { - BazelProtosProcessExecutionCodec { store } + ActionSerializer { store } } - fn digest_bytes(bytes: &Bytes) -> Digest { - let fingerprint = fs::Store::fingerprint_from_bytes_unsafe(&bytes); - Digest(fingerprint, bytes.len()) - } - - pub fn digest_message(message: &dyn GrpcioMessage) -> Result { - let bytes = message.write_to_bytes().map_err(|e| format!("{:?}", e))?; - Ok(Self::digest_bytes(&Bytes::from(bytes.as_slice()))) - } - - fn convert_digest(digest: Digest) -> bazel_protos::remote_execution::Digest { - let mut digest_proto = bazel_protos::remote_execution::Digest::new(); - let Digest(fingerprint, bytes_len) = digest; - digest_proto.set_hash(fingerprint.to_hex()); - digest_proto.set_size_bytes(bytes_len as i64); - digest_proto + pub fn convert_digest( + digest: &Digest, + ) -> bazel_protos::build::bazel::remote::execution::v2::Digest { + let Digest(fingerprint, len) = digest; + bazel_protos::build::bazel::remote::execution::v2::Digest { + hash: fingerprint.to_hex(), + size_bytes: *len as i64, + } } fn make_command( - req: CacheableExecuteProcessRequest, - ) -> Result { + req: &CacheableExecuteProcessRequest, + ) -> Result { let CacheableExecuteProcessRequest { req, cache_key_gen_version, } = req; - let mut command = bazel_protos::remote_execution::Command::new(); - command.set_arguments(protobuf::RepeatedField::from_vec(req.argv.clone())); - - for (ref name, ref value) in &req.env { - if name.as_str() == CACHE_KEY_GEN_VERSION_ENV_VAR_NAME { - return Err(format!( - "Cannot set env var with name {} as that is reserved for internal use by pants", - CACHE_KEY_GEN_VERSION_ENV_VAR_NAME - )); - } - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); - env.set_name(name.to_string()); - env.set_value(value.to_string()); - command.mut_environment_variables().push(env); + let arguments = req.argv.clone(); + + if req.env.contains_key(CACHE_KEY_GEN_VERSION_ENV_VAR_NAME) { + return Err(format!( + "Cannot set env var with name {} as that is reserved for internal use by pants", + CACHE_KEY_GEN_VERSION_ENV_VAR_NAME + )); } + let mut env_var_pairs: Vec<(String, String)> = req.env.clone().into_iter().collect(); if let Some(cache_key_gen_version) = cache_key_gen_version { - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); - env.set_name(CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_string()); - env.set_value(cache_key_gen_version.to_string()); - command.mut_environment_variables().push(env); + env_var_pairs.push(( + CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_string(), + cache_key_gen_version.to_string(), + )); } + let environment_variables: Vec<_> = env_var_pairs + .into_iter() + .map(|(name, value)| { + bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name, + value, + } + }) + .collect(); + let mut output_files = req .output_files .iter() @@ -236,7 +164,6 @@ impl BazelProtosProcessExecutionCodec { }) .collect::, String>>()?; output_files.sort(); - command.set_output_files(protobuf::RepeatedField::from_vec(output_files)); let mut output_directories = req .output_directories @@ -248,49 +175,73 @@ impl BazelProtosProcessExecutionCodec { }) .collect::, String>>()?; output_directories.sort(); - command.set_output_directories(protobuf::RepeatedField::from_vec(output_directories)); // Ideally, the JDK would be brought along as part of the input directory, but we don't currently // have support for that. The platform with which we're experimenting for remote execution // supports this property, and will symlink .jdk to a system-installed JDK: // https://github.com/twitter/scoot/pull/391 - if req.jdk_home.is_some() { - command.set_platform({ - let mut platform = bazel_protos::remote_execution::Platform::new(); - platform.mut_properties().push({ - let mut property = bazel_protos::remote_execution::Platform_Property::new(); - property.set_name("JDK_SYMLINK".to_owned()); - property.set_value(".jdk".to_owned()); - property - }); - platform - }); - } + let platform = if req.jdk_home.is_some() { + // This really should be req.jdk_home.map(|_| {...}), but that gives "cannot move out of + // borrowed content". + Some( + bazel_protos::build::bazel::remote::execution::v2::Platform { + properties: vec![ + bazel_protos::build::bazel::remote::execution::v2::platform::Property { + name: "JDK_SYMLINK".to_owned(), + value: ".jdk".to_owned(), + }, + ], + }, + ) + } else { + None + }; - Ok(command) + Ok(bazel_protos::build::bazel::remote::execution::v2::Command { + arguments, + environment_variables, + output_files, + output_directories, + platform, + working_directory: "".to_owned(), + }) + } + + fn encode_command_proto( + command: &bazel_protos::build::bazel::remote::execution::v2::Command, + ) -> Result { + fs::Store::encode_proto(command) + .map_err(|e| format!("Error serializing Command proto {:?}: {:?}", command, e)) } + /// ???/having the Command is necessary for making an ExecuteRequest proto, which we don't need in + /// this file. pub fn make_action_with_command( - req: CacheableExecuteProcessRequest, + req: &CacheableExecuteProcessRequest, ) -> Result< ( - bazel_protos::remote_execution::Action, - bazel_protos::remote_execution::Command, + bazel_protos::build::bazel::remote::execution::v2::Action, + bazel_protos::build::bazel::remote::execution::v2::Command, ), String, > { - let command = Self::make_command(req.clone())?; - let mut action = bazel_protos::remote_execution::Action::new(); - action.set_command_digest((&Self::digest_message(&command)?).into()); - action.set_input_root_digest((&req.req.input_files).into()); + let command = Self::make_command(&req)?; + let command_proto_bytes = Self::encode_command_proto(&command)?; + let action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(Self::convert_digest(&fs::Store::digest_bytes( + &command_proto_bytes, + ))), + input_root_digest: Some(Self::convert_digest(&req.req.input_files)), + ..bazel_protos::build::bazel::remote::execution::v2::Action::default() + }; Ok((action, command)) } fn extract_stdout( &self, - result: bazel_protos::remote_execution::ActionResult, + result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, ) -> BoxFuture { - if let Some(ref stdout_digest) = result.stdout_digest.into_option() { + if let Some(ref stdout_digest) = result.stdout_digest { let stdout_digest_result: Result = stdout_digest.into(); let stdout_digest = try_future!( stdout_digest_result.map_err(|err| format!("Error extracting stdout: {}", err)) @@ -314,7 +265,7 @@ impl BazelProtosProcessExecutionCodec { }) .to_boxed() } else { - let stdout_raw = result.stdout_raw; + let stdout_raw = Bytes::from(result.stdout_raw.clone()); let stdout_copy = stdout_raw.clone(); self .store @@ -327,9 +278,9 @@ impl BazelProtosProcessExecutionCodec { fn extract_stderr( &self, - result: bazel_protos::remote_execution::ActionResult, + result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, ) -> BoxFuture { - if let Some(ref stderr_digest) = result.stderr_digest.into_option() { + if let Some(ref stderr_digest) = result.stderr_digest { let stderr_digest_result: Result = stderr_digest.into(); let stderr_digest = try_future!( stderr_digest_result.map_err(|err| format!("Error extracting stderr: {}", err)) @@ -353,7 +304,7 @@ impl BazelProtosProcessExecutionCodec { }) .to_boxed() } else { - let stderr_raw = result.stderr_raw; + let stderr_raw = Bytes::from(result.stderr_raw.clone()); let stderr_copy = stderr_raw.clone(); self .store @@ -366,7 +317,7 @@ impl BazelProtosProcessExecutionCodec { fn extract_output_files( &self, - result: bazel_protos::remote_execution::ActionResult, + result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, ) -> BoxFuture { // Get Digests of output Directories. // Then we'll make a Directory for the output files, and merge them. @@ -404,7 +355,6 @@ impl BazelProtosProcessExecutionCodec { let output_file_path_buf = PathBuf::from(output_file.path); let digest = output_file .digest - .into_option() .ok_or_else(|| "No digest on remote execution output file".to_string())?; let digest: Result = (&digest).into(); path_map.insert(output_file_path_buf.clone(), digest?); @@ -466,58 +416,79 @@ impl BazelProtosProcessExecutionCodec { }) .to_boxed() } -} -/// ???/it's called "immediate" because it's a best-effort thing located locally (???) -pub trait ImmediateExecutionCache: Send + Sync { - fn record_process_result(&self, req: ProcessRequest, res: ProcessResult) - -> BoxFuture<(), String>; - - fn load_process_result(&self, req: ProcessRequest) -> BoxFuture, String>; -} - -/// ??? -#[derive(Clone)] -pub struct ActionCache { - store: fs::Store, - // NB: This could be an Arc>> if we ever need to - // add any further such codecs. This type is static in this struct, because the codec is required - // to produce specifically Action and ActionResult, because that is what is currently accepted by - // `store.record_process_result()`. - process_execution_codec: BazelProtosProcessExecutionCodec, -} + pub fn convert_request_to_action( + req: &CacheableExecuteProcessRequest, + ) -> Result { + let (action, _) = Self::make_action_with_command(req)?; + Ok(action) + } -impl ActionCache { - pub fn new(store: fs::Store) -> Self { - ActionCache { - store: store.clone(), - process_execution_codec: BazelProtosProcessExecutionCodec::new(store), + pub fn convert_result_to_action_result( + res: &CacheableExecuteProcessResult, + ) -> bazel_protos::build::bazel::remote::execution::v2::ActionResult { + bazel_protos::build::bazel::remote::execution::v2::ActionResult { + output_files: vec![], + output_directories: vec![ + bazel_protos::build::bazel::remote::execution::v2::OutputDirectory { + path: "".to_string(), + tree_digest: Some(Self::convert_digest(&res.output_directory)), + }, + ], + exit_code: res.exit_code, + stdout_raw: res.stdout.to_vec(), + stdout_digest: Some(Self::convert_digest(&fs::Store::digest_bytes(&res.stdout))), + stderr_raw: res.stderr.to_vec(), + stderr_digest: Some(Self::convert_digest(&fs::Store::digest_bytes(&res.stderr))), + execution_metadata: None, } } + + pub fn extract_action_result( + &self, + res: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, + ) -> BoxFuture { + let exit_code = res.exit_code; + self + .extract_stdout(&res) + .join(self.extract_stderr(&res)) + .join(self.extract_output_files(&res)) + .map( + move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { + stdout, + stderr, + exit_code, + output_directory, + }, + ) + .to_boxed() + } } impl ImmediateExecutionCache - for ActionCache + for ActionSerializer { fn record_process_result( &self, - req: CacheableExecuteProcessRequest, - res: CacheableExecuteProcessResult, + req: &CacheableExecuteProcessRequest, + res: &CacheableExecuteProcessResult, ) -> BoxFuture<(), String> { - let codec = self.process_execution_codec.clone(); - let converted_key_value_protos = codec - .convert_request(req) - .and_then(|req| codec.convert_response(res.clone()).map(|res| (req, res))); + let action_request = Self::convert_request_to_action(&req); + let action_result = Self::convert_result_to_action_result(&res); let store = self.store.clone(); - future::result(converted_key_value_protos) - .and_then(move |(action_request, action_result)| { + // TODO: I wish there was a shorthand syntax to extract multiple fields from a reference to a + // struct while cloning them. + let stdout = res.stdout.clone(); + let stderr = res.stderr.clone(); + future::result(action_request) + .and_then(move |action_request| { store - .store_file_bytes(res.stdout, true) - .join(store.store_file_bytes(res.stderr, true)) + .store_file_bytes(stdout, true) + .join(store.store_file_bytes(stderr, true)) .and_then(move |(_, _)| { // NB: We wait until the stdout and stderr digests have been successfully recorded, so // that we don't later attempt to read digests in the `action_result` which don't exist. - store.record_process_result(action_request, action_result) + store.record_process_result(&action_request, &action_result) }) .to_boxed() }) @@ -526,14 +497,17 @@ impl ImmediateExecutionCache BoxFuture, String> { - let codec = self.process_execution_codec.clone(); let store = self.store.clone(); - future::result(codec.convert_request(req)) - .and_then(move |action_proto| store.load_process_result(action_proto)) + let cache = self.clone(); + future::result(Self::convert_request_to_action(req)) + .and_then(move |action_proto| store.load_process_result(&action_proto)) .and_then(move |maybe_action_result| match maybe_action_result { - Some(action_result) => codec.extract_response(action_result).map(Some).to_boxed(), + Some(action_result) => cache + .extract_action_result(&action_result) + .map(Some) + .to_boxed(), None => future::result(Ok(None)).to_boxed(), }) .to_boxed() @@ -558,8 +532,8 @@ impl CachingCommandRunner { store: fs::Store, cache_key_gen_version: Option, ) -> Self { - let action_cache = ActionCache::new(store); - let boxed_cache = Box::new(action_cache) + let action_serializer = ActionSerializer::new(store); + let boxed_cache = Box::new(action_serializer) as Box< dyn ImmediateExecutionCache, >; @@ -588,7 +562,7 @@ impl CommandRunner for CachingCommandRunner { let cache = self.cache.clone(); let inner = self.inner.clone(); cache - .load_process_result(cacheable_request.clone()) + .load_process_result(&cacheable_request) .and_then(move |cache_fetch| match cache_fetch { // We have a cache hit! Some(cached_execution_result) => { @@ -606,12 +580,11 @@ impl CommandRunner for CachingCommandRunner { debug!("uncached execution for request {:?}: {:?}", req, res); let cacheable_process_result = res.into_cacheable(); cache - .record_process_result(cacheable_request.clone(), cacheable_process_result.clone()) + .record_process_result(&cacheable_request, &cacheable_process_result) .map(move |()| { debug!( "request {:?} should now be cached as {:?}", - cacheable_request.clone(), - cacheable_process_result.clone() + &cacheable_request, &cacheable_process_result, ); cacheable_process_result }) @@ -628,7 +601,7 @@ impl CommandRunner for CachingCommandRunner { #[cfg(test)] mod tests { use super::{ - ActionCache, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, + ActionSerializer, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, CachingCommandRunner, CommandRunner, ExecuteProcessRequest, ImmediateExecutionCache, }; use futures::future::Future; @@ -661,25 +634,25 @@ mod tests { ])); let store_dir = TempDir::new().unwrap(); let work_dir = TempDir::new().unwrap(); - let (_, base_runner, action_cache) = cache_in_dir(store_dir.path(), work_dir.path()); + let (_, base_runner, action_serializer) = cache_in_dir(store_dir.path(), work_dir.path()); let cacheable_perl = CacheableExecuteProcessRequest { req: random_perl.clone(), cache_key_gen_version: None, }; assert_eq!( Ok(None), - action_cache + action_serializer .load_process_result(cacheable_perl.clone()) .wait() ); - let caching_runner = make_caching_runner(base_runner.clone(), action_cache.clone(), None); + let caching_runner = make_caching_runner(base_runner.clone(), action_serializer.clone(), None); let process_result = caching_runner.run(random_perl.clone()).wait().unwrap(); assert_eq!(0, process_result.exit_code); // A "cacheable" process execution result won't have e.g. the number of attempts that the // process was tried, for idempotency, but everything else should be the same. assert_eq!( process_result.clone().into_cacheable(), - action_cache + action_serializer .load_process_result(cacheable_perl) .wait() .unwrap() @@ -704,19 +677,22 @@ mod tests { }; assert_eq!( Ok(None), - action_cache + action_serializer .load_process_result(new_cacheable_perl.clone()) .wait() ); - let new_caching_runner = - make_caching_runner(base_runner.clone(), action_cache.clone(), Some(new_key)); + let new_caching_runner = make_caching_runner( + base_runner.clone(), + action_serializer.clone(), + Some(new_key), + ); let new_process_result = new_caching_runner.run(random_perl.clone()).wait().unwrap(); assert_eq!(0, new_process_result.exit_code); // The new `cache_key_gen_version` is propagated to the requests made against the // CachingCommandRunner. assert_eq!( new_process_result.clone().into_cacheable(), - action_cache + action_serializer .load_process_result(new_cacheable_perl) .wait() .unwrap() @@ -738,13 +714,13 @@ mod tests { }; assert_eq!( Ok(None), - action_cache + action_serializer .load_process_result(second_cache_string_perl.clone()) .wait() ); let second_string_caching_runner = make_caching_runner( base_runner.clone(), - action_cache.clone(), + action_serializer.clone(), Some(second_string_key), ); let second_string_process_result = second_string_caching_runner @@ -754,7 +730,7 @@ mod tests { assert_eq!(0, second_string_process_result.exit_code); assert_eq!( second_string_process_result.clone().into_cacheable(), - action_cache + action_serializer .load_process_result(second_cache_string_perl) .wait() .unwrap() @@ -786,23 +762,23 @@ mod tests { fn cache_in_dir( store_dir: &Path, work_dir: &Path, - ) -> (fs::Store, crate::local::CommandRunner, ActionCache) { + ) -> (fs::Store, crate::local::CommandRunner, ActionSerializer) { let pool = Arc::new(fs::ResettablePool::new("test-pool-".to_owned())); let store = fs::Store::local_only(store_dir, pool.clone()).unwrap(); - let action_cache = ActionCache::new(store.clone()); + let action_serializer = ActionSerializer::new(store.clone()); let base_runner = crate::local::CommandRunner::new(store.clone(), pool, work_dir.to_path_buf(), true); - (store, base_runner, action_cache) + (store, base_runner, action_serializer) } fn make_caching_runner( base_runner: crate::local::CommandRunner, - action_cache: ActionCache, + action_serializer: ActionSerializer, cache_key_gen_version: Option, ) -> CachingCommandRunner { CachingCommandRunner::new( Box::new(base_runner) as Box, - Box::new(action_cache) + Box::new(action_serializer) as Box< dyn ImmediateExecutionCache< CacheableExecuteProcessRequest, diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 32aa4746ea5..8022879a2eb 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -44,9 +44,8 @@ pub mod cached_execution; pub mod local; pub mod remote; pub use crate::cached_execution::{ - ActionCache, BazelProtosProcessExecutionCodec, CacheableExecuteProcessRequest, - CacheableExecuteProcessResult, CachingCommandRunner, ImmediateExecutionCache, - SerializableProcessExecutionCodec, + ActionSerializer, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, + CachingCommandRunner, ImmediateExecutionCache, }; /// diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 7011b2e6684..f1c5cfccf1f 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -2,21 +2,19 @@ use std::time::{Duration, Instant}; use bazel_protos; use boxfuture::{try_future, BoxFuture, Boxable}; -use bytes::Bytes; use fs::{self, Store}; use futures::{future, Future, Stream}; use futures_timer::Delay; use hashing::{Digest, Fingerprint}; use log::{debug, trace, warn}; use parking_lot::Mutex; -use prost::Message; +use prost::{self, Message}; use protobuf::{self, Message as GrpcioMessage, ProtobufEnum}; use time; use super::{ - BazelProtosProcessExecutionCodec, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, - ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult, - SerializableProcessExecutionCodec, + ActionSerializer, CacheableExecuteProcessRequest, ExecuteProcessRequest, ExecutionStats, + FallibleExecuteProcessResult, }; use std; use std::cmp::min; @@ -54,7 +52,7 @@ pub struct CommandRunner { clients: futures::future::Shared>, store: Store, futures_timer_thread: resettable::Resettable, - process_execution_codec: BazelProtosProcessExecutionCodecV2, + action_serializer: ActionSerializer, } #[derive(Debug, PartialEq)] @@ -118,207 +116,34 @@ impl CommandRunner { } #[derive(Clone)] -pub struct BazelProcessExecutionRequestV2 { - action: bazel_protos::remote_execution::Action, - command: bazel_protos::remote_execution::Command, +pub struct BazelProcessExecutionRequest { + action: bazel_protos::build::bazel::remote::execution::v2::Action, + command: bazel_protos::build::bazel::remote::execution::v2::Command, execute_request: bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, } -#[derive(Clone)] -pub struct BazelProtosProcessExecutionCodecV2 { - inner: BazelProtosProcessExecutionCodec, - instance_name: Option, -} - -impl BazelProtosProcessExecutionCodecV2 { - fn new(store: fs::Store, instance_name: Option) -> Self { - let inner = BazelProtosProcessExecutionCodec::new(store); - BazelProtosProcessExecutionCodecV2 { - inner, - instance_name, - } - } - - fn convert_digest( - digest: bazel_protos::build::bazel::remote::execution::v2::Digest, - ) -> bazel_protos::remote_execution::Digest { - let mut resulting_digest = bazel_protos::remote_execution::Digest::new(); - resulting_digest.set_hash(digest.hash); - resulting_digest.set_size_bytes(digest.size_bytes); - resulting_digest - } - - fn convert_output_file( - output_file: bazel_protos::build::bazel::remote::execution::v2::OutputFile, - ) -> bazel_protos::remote_execution::OutputFile { - let mut resulting_output_file = bazel_protos::remote_execution::OutputFile::new(); - resulting_output_file.set_path(output_file.path); - if let Some(digest) = output_file.digest.map(Self::convert_digest) { - resulting_output_file.set_digest(digest); - } - resulting_output_file.set_is_executable(output_file.is_executable); - resulting_output_file - } - - fn convert_output_directory( - output_dir: bazel_protos::build::bazel::remote::execution::v2::OutputDirectory, - ) -> bazel_protos::remote_execution::OutputDirectory { - let mut resulting_output_dir = bazel_protos::remote_execution::OutputDirectory::new(); - resulting_output_dir.set_path(output_dir.path); - if let Some(digest) = output_dir.tree_digest.map(Self::convert_digest) { - resulting_output_dir.set_tree_digest(digest); - } - resulting_output_dir - } - - fn convert_timestamp(timestamp: prost_types::Timestamp) -> protobuf::well_known_types::Timestamp { - let mut resulting_timestamp = protobuf::well_known_types::Timestamp::new(); - resulting_timestamp.set_seconds(timestamp.seconds); - resulting_timestamp.set_nanos(timestamp.nanos); - resulting_timestamp - } - - fn convert_execution_metadata( - exec_metadata: bazel_protos::build::bazel::remote::execution::v2::ExecutedActionMetadata, - ) -> bazel_protos::remote_execution::ExecutedActionMetadata { - let mut resulting_exec_metadata = bazel_protos::remote_execution::ExecutedActionMetadata::new(); - resulting_exec_metadata.set_worker(exec_metadata.worker); - if let Some(queued_timestamp) = exec_metadata.queued_timestamp.map(Self::convert_timestamp) { - resulting_exec_metadata.set_queued_timestamp(queued_timestamp); - } - if let Some(worker_start_timestamp) = exec_metadata - .worker_start_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_worker_start_timestamp(worker_start_timestamp); - } - if let Some(worker_completed_timestamp) = exec_metadata - .worker_completed_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_worker_completed_timestamp(worker_completed_timestamp); - } - if let Some(input_fetch_start_timestamp) = exec_metadata - .input_fetch_start_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_input_fetch_start_timestamp(input_fetch_start_timestamp); - } - if let Some(input_fetch_completed_timestamp) = exec_metadata - .input_fetch_completed_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_input_fetch_completed_timestamp(input_fetch_completed_timestamp); - } - if let Some(execution_start_timestamp) = exec_metadata - .execution_start_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_execution_start_timestamp(execution_start_timestamp); - } - if let Some(execution_completed_timestamp) = exec_metadata - .execution_completed_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_execution_completed_timestamp(execution_completed_timestamp); - } - if let Some(output_upload_start_timestamp) = exec_metadata - .output_upload_start_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata.set_output_upload_start_timestamp(output_upload_start_timestamp); - } - if let Some(output_upload_completed_timestamp) = exec_metadata - .output_upload_completed_timestamp - .map(Self::convert_timestamp) - { - resulting_exec_metadata - .set_output_upload_completed_timestamp(output_upload_completed_timestamp); - } - resulting_exec_metadata - } - - fn convert_action_result( - action_result: bazel_protos::build::bazel::remote::execution::v2::ActionResult, - ) -> bazel_protos::remote_execution::ActionResult { - let mut resulting_action_result = bazel_protos::remote_execution::ActionResult::new(); - resulting_action_result.set_output_files(protobuf::RepeatedField::from_vec( - action_result - .output_files - .iter() - .cloned() - .map(Self::convert_output_file) - .collect(), - )); - resulting_action_result.set_output_directories(protobuf::RepeatedField::from_vec( - action_result - .output_directories - .iter() - .cloned() - .map(Self::convert_output_directory) - .collect(), - )); - resulting_action_result.set_exit_code(action_result.exit_code); - resulting_action_result.set_stdout_raw(Bytes::from(action_result.stdout_raw)); - if let Some(digest) = action_result.stdout_digest.map(Self::convert_digest) { - resulting_action_result.set_stdout_digest(digest); - } - resulting_action_result.set_stderr_raw(Bytes::from(action_result.stderr_raw)); - if let Some(digest) = action_result.stderr_digest.map(Self::convert_digest) { - resulting_action_result.set_stderr_digest(digest); - } - if let Some(execution_metadata) = action_result - .execution_metadata - .map(Self::convert_execution_metadata) - { - resulting_action_result.set_execution_metadata(execution_metadata); - } - resulting_action_result - } -} - -impl - SerializableProcessExecutionCodec< - CacheableExecuteProcessRequest, - BazelProcessExecutionRequestV2, - CacheableExecuteProcessResult, - bazel_protos::build::bazel::remote::execution::v2::ActionResult, - String, - > for BazelProtosProcessExecutionCodecV2 -{ - fn convert_request( - &self, - req: CacheableExecuteProcessRequest, - ) -> Result { - let (action, command) = BazelProtosProcessExecutionCodec::make_action_with_command(req)?; +impl BazelProcessExecutionRequest { + fn convert_execute_request( + req: &CacheableExecuteProcessRequest, + instance_name: &Option, + ) -> Result { + let (action, command) = ActionSerializer::make_action_with_command(&req)?; + let action_proto_bytes = &fs::Store::encode_action_proto(&action)?; let execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some((&BazelProtosProcessExecutionCodec::digest_message(&action)?).into()), + action_digest: Some(ActionSerializer::convert_digest(&fs::Store::digest_bytes( + &action_proto_bytes, + ))), skip_cache_lookup: false, - instance_name: self.instance_name.clone().unwrap_or_default(), + instance_name: instance_name.clone().unwrap_or_default(), execution_policy: None, results_cache_policy: None, }; - Ok(BazelProcessExecutionRequestV2 { + Ok(BazelProcessExecutionRequest { action, command, execute_request, }) } - - fn convert_response( - &self, - _res: CacheableExecuteProcessResult, - ) -> Result { - panic!("converting from a cacheable process request to a v2 ActionResult is not yet supported"); - } - - fn extract_response( - &self, - serializable_response: bazel_protos::build::bazel::remote::execution::v2::ActionResult, - ) -> BoxFuture { - let action_result_v1 = Self::convert_action_result(serializable_response); - self.inner.extract_response(action_result_v1) - } } impl super::CommandRunner for CommandRunner { @@ -347,9 +172,10 @@ impl super::CommandRunner for CommandRunner { let store = self.store.clone(); let cacheable_execute_process_request = CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); - let execute_request_result = self - .process_execution_codec - .convert_request(cacheable_execute_process_request); + let execute_request_result = BazelProcessExecutionRequest::convert_execute_request( + &cacheable_execute_process_request, + &self.instance_name, + ); let ExecuteProcessRequest { description, @@ -361,7 +187,7 @@ impl super::CommandRunner for CommandRunner { let description2 = description.clone(); match execute_request_result { - Ok(BazelProcessExecutionRequestV2 { + Ok(BazelProcessExecutionRequest { action, command, execute_request, @@ -375,8 +201,8 @@ impl super::CommandRunner for CommandRunner { let mut history = ExecutionHistory::default(); self - .store_proto_locally(&command) - .join(self.store_proto_locally(&action)) + .store_prost_proto_locally(&command) + .join(self.store_prost_proto_locally(&action)) .and_then(move |(command_digest, action_digest)| { store2.ensure_remote_has_recursive(vec![command_digest, action_digest, input_files]) }) @@ -598,7 +424,7 @@ impl CommandRunner { clients, store: store.clone(), futures_timer_thread, - process_execution_codec: BazelProtosProcessExecutionCodecV2::new(store, instance_name), + action_serializer: ActionSerializer::new(store.clone()), }) } @@ -612,17 +438,15 @@ impl CommandRunner { request } - fn store_proto_locally( + fn store_prost_proto_locally( &self, proto: &P, ) -> impl Future { let store = self.store.clone(); future::done( - proto - .write_to_bytes() - .map_err(|e| format!("Error serializing proto {:?}", e)), + fs::Store::encode_proto(proto).map_err(|e| format!("Error serializing proto {:?}", e)), ) - .and_then(move |command_bytes| store.store_file_bytes(Bytes::from(command_bytes), true)) + .and_then(move |command_bytes| store.store_file_bytes(command_bytes, true)) .map_err(|e| format!("Error saving proto to local store: {:?}", e)) } @@ -706,12 +530,12 @@ impl CommandRunner { if status.code == bazel_protos::google::rpc::Code::Ok.into() { if let Some(result) = maybe_result { return self - .process_execution_codec - .extract_response(result.clone()) + .action_serializer + .extract_action_result(&result) .map(|cacheable_result| cacheable_result.with_execution_attempts(execution_attempts)) .map_err(move |err| { ExecutionError::Fatal(format!( - "error deocding process result {:?}: {:?}", + "error decoding process result {:?}: {:?}", result, err )) }) @@ -882,7 +706,7 @@ mod tests { use super::super::CommandRunner as CommandRunnerTrait; use super::{ - BazelProcessExecutionRequestV2, BazelProtosProcessExecutionCodec, + BazelProcessExecutionRequest, BazelProtosProcessExecutionCodec, BazelProtosProcessExecutionCodecV2, CacheableExecuteProcessRequest, CommandRunner, ExecuteProcessRequest, ExecutionError, ExecutionHistory, FallibleExecuteProcessResult, }; @@ -930,11 +754,12 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::remote_execution::Command::new(); + let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); want_command.mut_arguments().push("/bin/echo".to_owned()); want_command.mut_arguments().push("yo".to_owned()); want_command.mut_environment_variables().push({ - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env.set_name("SOME".to_owned()); env.set_value("value".to_owned()); env @@ -949,7 +774,7 @@ mod tests { .mut_output_directories() .push("directory/name".to_owned()); - let mut want_action = bazel_protos::remote_execution::Action::new(); + let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); want_action.set_command_digest( (&Digest( Fingerprint::from_hex_string( @@ -1005,11 +830,12 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::remote_execution::Command::new(); + let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); want_command.mut_arguments().push("/bin/echo".to_owned()); want_command.mut_arguments().push("yo".to_owned()); want_command.mut_environment_variables().push({ - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env.set_name("SOME".to_owned()); env.set_value("value".to_owned()); env @@ -1024,7 +850,7 @@ mod tests { .mut_output_directories() .push("directory/name".to_owned()); - let mut want_action = bazel_protos::remote_execution::Action::new(); + let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); want_action.set_command_digest( (&Digest( Fingerprint::from_hex_string( @@ -1094,17 +920,19 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::remote_execution::Command::new(); + let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); want_command.mut_arguments().push("/bin/echo".to_owned()); want_command.mut_arguments().push("yo".to_owned()); want_command.mut_environment_variables().push({ - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env.set_name("SOME".to_owned()); env.set_value("value".to_owned()); env }); want_command.mut_environment_variables().push({ - let mut env = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env.set_name(crate::cached_execution::CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_owned()); env.set_value("meep".to_owned()); env @@ -1119,7 +947,7 @@ mod tests { .mut_output_directories() .push("directory/name".to_owned()); - let mut want_action = bazel_protos::remote_execution::Action::new(); + let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); want_action.set_command_digest( (&Digest( Fingerprint::from_hex_string( @@ -1166,7 +994,7 @@ mod tests { jdk_home: Some(PathBuf::from("/tmp")), }; - let mut want_command = bazel_protos::remote_execution::Command::new(); + let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); want_command.mut_arguments().push("/bin/echo".to_owned()); want_command.mut_arguments().push("yo".to_owned()); want_command.mut_platform().mut_properties().push({ @@ -1176,7 +1004,7 @@ mod tests { property }); - let mut want_action = bazel_protos::remote_execution::Action::new(); + let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); want_action.set_command_digest( (&Digest( Fingerprint::from_hex_string( @@ -1282,7 +1110,7 @@ mod tests { } #[test] - fn extract_response_with_digest_stdout() { + fn extract_action_result_with_digest_stdout() { let op_name = "gimme-foo".to_string(); let testdata = TestData::roland(); let testdata_empty = TestData::empty(); @@ -1311,7 +1139,7 @@ mod tests { } #[test] - fn extract_response_with_digest_stderr() { + fn extract_action_result_with_digest_stderr() { let op_name = "gimme-foo".to_string(); let testdata = TestData::roland(); let testdata_empty = TestData::empty(); @@ -2133,21 +1961,23 @@ mod tests { #[test] fn digest_command() { - let mut command = bazel_protos::remote_execution::Command::new(); + let mut command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); command.mut_arguments().push("/bin/echo".to_string()); command.mut_arguments().push("foo".to_string()); - let mut env1 = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env1 = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env1.set_name("A".to_string()); env1.set_value("a".to_string()); command.mut_environment_variables().push(env1); - let mut env2 = bazel_protos::remote_execution::Command_EnvironmentVariable::new(); + let mut env2 = + bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); env2.set_name("B".to_string()); env2.set_value("b".to_string()); command.mut_environment_variables().push(env2); - let digest = BazelProtosProcessExecutionCodec::digest_message(&command).unwrap(); + let digest = ActionSerializer::digest_proto(&command).unwrap(); assert_eq!( &digest.0.to_hex(), @@ -2578,7 +2408,7 @@ mod tests { let codec = codec_from_store(store, None); let mut runtime = tokio::runtime::Runtime::new().unwrap(); - let result = runtime.block_on(codec.extract_response(result.clone())); + let result = runtime.block_on(codec.extract_action_result(result.clone())); runtime.shutdown_now().wait().unwrap(); result .map(|res| res.output_directory) @@ -2666,9 +2496,9 @@ mod tests { req: ExecuteProcessRequest, instance_name: Option, cache_key_gen_version: Option, - ) -> Result { + ) -> Result { let codec = codec(dir, instance_name); - codec.convert_request(CacheableExecuteProcessRequest::new( + codec.convert_request_to_action(CacheableExecuteProcessRequest::new( req, cache_key_gen_version, )) @@ -2680,8 +2510,8 @@ mod tests { cache_key_gen_version: &Option, ) -> Result< ( - bazel_protos::remote_execution::Action, - bazel_protos::remote_execution::Command, + bazel_protos::build::bazel::remote::execution::v2::Action, + bazel_protos::build::bazel::remote::execution::v2::Command, bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, ), String, @@ -2694,7 +2524,7 @@ mod tests { cache_key_gen_version.clone(), ) .map( - |BazelProcessExecutionRequestV2 { + |BazelProcessExecutionRequest { action, command, execute_request, From 617f32c6ac5bc4e6c8ea24048309c7edecac9346 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 17:45:17 -0800 Subject: [PATCH 05/10] make every test but mine pass --- .../process_execution/src/cached_execution.rs | 14 +- .../engine/process_execution/src/remote.rs | 781 +++++++++--------- 2 files changed, 380 insertions(+), 415 deletions(-) diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index 832fe4aeeb5..b27cea3e784 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -207,7 +207,7 @@ impl ActionSerializer { }) } - fn encode_command_proto( + pub fn encode_command_proto( command: &bazel_protos::build::bazel::remote::execution::v2::Command, ) -> Result { fs::Store::encode_proto(command) @@ -642,7 +642,7 @@ mod tests { assert_eq!( Ok(None), action_serializer - .load_process_result(cacheable_perl.clone()) + .load_process_result(&cacheable_perl) .wait() ); let caching_runner = make_caching_runner(base_runner.clone(), action_serializer.clone(), None); @@ -653,7 +653,7 @@ mod tests { assert_eq!( process_result.clone().into_cacheable(), action_serializer - .load_process_result(cacheable_perl) + .load_process_result(&cacheable_perl) .wait() .unwrap() .unwrap() @@ -678,7 +678,7 @@ mod tests { assert_eq!( Ok(None), action_serializer - .load_process_result(new_cacheable_perl.clone()) + .load_process_result(&new_cacheable_perl) .wait() ); let new_caching_runner = make_caching_runner( @@ -693,7 +693,7 @@ mod tests { assert_eq!( new_process_result.clone().into_cacheable(), action_serializer - .load_process_result(new_cacheable_perl) + .load_process_result(&new_cacheable_perl) .wait() .unwrap() .unwrap() @@ -715,7 +715,7 @@ mod tests { assert_eq!( Ok(None), action_serializer - .load_process_result(second_cache_string_perl.clone()) + .load_process_result(&second_cache_string_perl) .wait() ); let second_string_caching_runner = make_caching_runner( @@ -731,7 +731,7 @@ mod tests { assert_eq!( second_string_process_result.clone().into_cacheable(), action_serializer - .load_process_result(second_cache_string_perl) + .load_process_result(&second_cache_string_perl) .wait() .unwrap() .unwrap() diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index f1c5cfccf1f..0b6199fc68f 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -115,7 +115,7 @@ impl CommandRunner { } } -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct BazelProcessExecutionRequest { action: bazel_protos::build::bazel::remote::execution::v2::Action, command: bazel_protos::build::bazel::remote::execution::v2::Command, @@ -123,6 +123,18 @@ pub struct BazelProcessExecutionRequest { } impl BazelProcessExecutionRequest { + fn new( + action: bazel_protos::build::bazel::remote::execution::v2::Action, + command: bazel_protos::build::bazel::remote::execution::v2::Command, + execute_request: bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, + ) -> Self { + BazelProcessExecutionRequest { + action, + command, + execute_request, + } + } + fn convert_execute_request( req: &CacheableExecuteProcessRequest, instance_name: &Option, @@ -138,14 +150,25 @@ impl BazelProcessExecutionRequest { execution_policy: None, results_cache_policy: None, }; - Ok(BazelProcessExecutionRequest { + Ok(BazelProcessExecutionRequest::new( action, command, execute_request, - }) + )) } } +#[cfg(test)] +impl PartialEq for BazelProcessExecutionRequest { + fn eq(&self, other: &Self) -> bool { + (&self.action, &self.command, &self.execute_request) + == (&other.action, &other.command, &other.execute_request) + } +} + +#[cfg(test)] +impl Eq for BazelProcessExecutionRequest {} + impl super::CommandRunner for CommandRunner { /// /// Runs a command via a gRPC service implementing the Bazel Remote Execution API @@ -167,184 +190,20 @@ impl super::CommandRunner for CommandRunner { /// TODO: Request jdk_home be created if set. /// fn run(&self, req: ExecuteProcessRequest) -> BoxFuture { - let clients = self.clients.clone(); - - let store = self.store.clone(); - let cacheable_execute_process_request = + let cacheable_input_request = CacheableExecuteProcessRequest::new(req.clone(), self.cache_key_gen_version.clone()); - let execute_request_result = BazelProcessExecutionRequest::convert_execute_request( - &cacheable_execute_process_request, + let converted_request_protos = BazelProcessExecutionRequest::convert_execute_request( + &cacheable_input_request, &self.instance_name, ); - - let ExecuteProcessRequest { - description, - timeout, - input_files, - .. - } = req; - - let description2 = description.clone(); - - match execute_request_result { - Ok(BazelProcessExecutionRequest { - action, - command, - execute_request, - }) => { - let command_runner = self.clone(); - let command_runner2 = self.clone(); - let execute_request2 = execute_request.clone(); - let futures_timer_thread = self.futures_timer_thread.clone(); - - let store2 = store.clone(); - let mut history = ExecutionHistory::default(); - - self - .store_prost_proto_locally(&command) - .join(self.store_prost_proto_locally(&action)) - .and_then(move |(command_digest, action_digest)| { - store2.ensure_remote_has_recursive(vec![command_digest, action_digest, input_files]) - }) - .and_then(move |summary| { - history.current_attempt += summary; - trace!( - "Executing remotely request: {:?} (command: {:?})", - execute_request, - command - ); - command_runner - .oneshot_execute(execute_request) - .join(future::ok(history)) - }) - .and_then(move |(operation, history)| { - let start_time = Instant::now(); - - future::loop_fn( - (history, operation, 0), - move |(mut history, operation, iter_num)| { - let description = description.clone(); - - let execute_request2 = execute_request2.clone(); - let store = store.clone(); - let clients = clients.clone(); - let command_runner2 = command_runner2.clone(); - let command_runner3 = command_runner2.clone(); - let futures_timer_thread = futures_timer_thread.clone(); - let f = command_runner2.extract_execute_response(operation, &mut history); - f.map(future::Loop::Break).or_else(move |value| { - match value { - ExecutionError::Fatal(err) => future::err(err).to_boxed(), - ExecutionError::MissingDigests(missing_digests) => { - let ExecutionHistory { - mut attempts, - current_attempt, - } = history; - - trace!( - "Server reported missing digests ({:?}); trying to upload: {:?}", - current_attempt, - missing_digests, - ); - - attempts.push(current_attempt); - let history = ExecutionHistory { - attempts, - current_attempt: ExecutionStats::default(), - }; - - let execute_request = execute_request2.clone(); - store - .ensure_remote_has_recursive(missing_digests) - .and_then(move |summary| { - let mut history = history; - history.current_attempt += summary; - command_runner2 - .oneshot_execute(execute_request) - .join(future::ok(history)) - }) - // Reset `iter_num` on `MissingDigests` - .map(|(operation, history)| future::Loop::Continue((history, operation, 0))) - .to_boxed() - } - ExecutionError::NotFinished(operation_name) => { - let operation_name2 = operation_name.clone(); - let operation_request = - bazel_protos::google::longrunning::GetOperationRequest { - name: operation_name.clone(), - }; - - let backoff_period = min( - CommandRunner::BACKOFF_MAX_WAIT_MILLIS, - (1 + iter_num) * CommandRunner::BACKOFF_INCR_WAIT_MILLIS, - ); - - // take the grpc result and cancel the op if too much time has passed. - let elapsed = start_time.elapsed(); - - if elapsed > timeout { - future::err(format!( - "Exceeded time out of {:?} with {:?} for operation {}, {}", - timeout, elapsed, operation_name, description - )) - .to_boxed() - } else { - // maybe the delay here should be the min of remaining time and the backoff period - Delay::new_handle( - Instant::now() + Duration::from_millis(backoff_period), - futures_timer_thread.with(|thread| thread.handle()), - ) - .map_err(move |e| { - format!( - "Future-Delay errored at operation result polling for {}, {}: {}", - operation_name, description, e - ) - }) - .and_then(move |_| { - clients - .map_err(|err| format!("{}", err)) - .and_then(move |clients| { - clients - .operations_client - .lock() - .get_operation(command_runner3.make_request(operation_request)) - .map(|r| r.into_inner()) - .or_else(move |err| { - rpcerror_recover_cancelled(operation_name2, err) - }) - .map_err(towergrpcerror_to_string) - }) - .map(OperationOrStatus::Operation) - .map(move |operation| { - future::Loop::Continue((history, operation, iter_num + 1)) - }) - .to_boxed() - }) - .to_boxed() - } - } - } - }) - }, - ) - }) - .map(move |resp| { - let mut attempts = String::new(); - for (i, attempt) in resp.execution_attempts.iter().enumerate() { - attempts += &format!("\nAttempt {}: {:?}", i, attempt); - } - debug!( - "Finished remote exceution of {} after {} attempts: Stats: {}", - description2, - resp.execution_attempts.len(), - attempts - ); - resp - }) + let command_runner = self.clone(); + future::result(converted_request_protos) + .and_then(move |action_request| { + command_runner + .execute_request(req, action_request.clone()) .to_boxed() - } - Err(err) => future::err(err).to_boxed(), - } + }) + .to_boxed() } } @@ -438,7 +297,7 @@ impl CommandRunner { request } - fn store_prost_proto_locally( + fn store_proto_locally( &self, proto: &P, ) -> impl Future { @@ -632,6 +491,176 @@ impl CommandRunner { } .to_boxed() } + + fn execute_request( + &self, + input_execution_request: ExecuteProcessRequest, + serializable_proto_request: BazelProcessExecutionRequest, + ) -> BoxFuture { + let ExecuteProcessRequest { + input_files, + description, + timeout, + .. + } = input_execution_request; + let BazelProcessExecutionRequest { + action, + command, + execute_request, + } = serializable_proto_request; + + let command_runner = self.clone(); + let command_runner2 = self.clone(); + let execute_request2 = execute_request.clone(); + let description2 = description.clone(); + let futures_timer_thread = self.futures_timer_thread.clone(); + + let store = self.store.clone(); + let store2 = self.store.clone(); + let clients = self.clients.clone(); + + let mut history = ExecutionHistory::default(); + + self + .store_proto_locally(&command) + .join(self.store_proto_locally(&action)) + .and_then(move |(command_digest, action_digest)| { + store.ensure_remote_has_recursive(vec![command_digest, action_digest, input_files]) + }) + .and_then(move |summary| { + history.current_attempt += summary; + trace!( + "Executing remotely request: {:?} (command: {:?})", + execute_request, + command + ); + command_runner + .oneshot_execute(execute_request) + .join(future::ok(history)) + }) + .and_then(move |(operation, history)| { + let start_time = Instant::now(); + + future::loop_fn( + (history, operation, 0), + move |(mut history, operation, iter_num)| { + let description = description2.clone(); + + let execute_request2 = execute_request2.clone(); + let store = store2.clone(); + let clients = clients.clone(); + let command_runner2 = command_runner2.clone(); + let command_runner3 = command_runner2.clone(); + let futures_timer_thread = futures_timer_thread.clone(); + let f = command_runner2.extract_execute_response(operation, &mut history); + f.map(future::Loop::Break).or_else(move |value| { + match value { + ExecutionError::Fatal(err) => future::err(err).to_boxed(), + ExecutionError::MissingDigests(missing_digests) => { + let ExecutionHistory { + mut attempts, + current_attempt, + } = history; + + trace!( + "Server reported missing digests ({:?}); trying to upload: {:?}", + current_attempt, + missing_digests, + ); + + attempts.push(current_attempt); + let history = ExecutionHistory { + attempts, + current_attempt: ExecutionStats::default(), + }; + + let execute_request = execute_request2.clone(); + store + .ensure_remote_has_recursive(missing_digests) + .and_then(move |summary| { + let mut history = history; + history.current_attempt += summary; + command_runner2 + .oneshot_execute(execute_request) + .join(future::ok(history)) + }) + // Reset `iter_num` on `MissingDigests` + .map(|(operation, history)| future::Loop::Continue((history, operation, 0))) + .to_boxed() + } + ExecutionError::NotFinished(operation_name) => { + let operation_name2 = operation_name.clone(); + let operation_request = bazel_protos::google::longrunning::GetOperationRequest { + name: operation_name.clone(), + }; + + let backoff_period = min( + CommandRunner::BACKOFF_MAX_WAIT_MILLIS, + (1 + iter_num) * CommandRunner::BACKOFF_INCR_WAIT_MILLIS, + ); + + // take the grpc result and cancel the op if too much time has passed. + let elapsed = start_time.elapsed(); + + if elapsed > timeout { + future::err(format!( + "Exceeded time out of {:?} with {:?} for operation {}, {}", + timeout, elapsed, operation_name, description + )) + .to_boxed() + } else { + // maybe the delay here should be the min of remaining time and the backoff period + Delay::new_handle( + Instant::now() + Duration::from_millis(backoff_period), + futures_timer_thread.with(|thread| thread.handle()), + ) + .map_err(move |e| { + format!( + "Future-Delay errored at operation result polling for {}, {}: {}", + operation_name, description, e + ) + }) + .and_then(move |_| { + clients + .map_err(|err| format!("{}", err)) + .and_then(move |clients| { + clients + .operations_client + .lock() + .get_operation(command_runner3.make_request(operation_request)) + .map(|r| r.into_inner()) + .or_else(move |err| rpcerror_recover_cancelled(operation_name2, err)) + .map_err(towergrpcerror_to_string) + }) + .map(OperationOrStatus::Operation) + .map(move |operation| { + future::Loop::Continue((history, operation, iter_num + 1)) + }) + .to_boxed() + }) + .to_boxed() + } + } + } + }) + }, + ) + }) + .map(move |resp| { + let mut attempts = String::new(); + for (i, attempt) in resp.execution_attempts.iter().enumerate() { + attempts += &format!("\nAttempt {}: {:?}", i, attempt); + } + debug!( + "Finished remote exceution of {} after {} attempts: Stats: {}", + description, + resp.execution_attempts.len(), + attempts + ); + resp + }) + .to_boxed() + } } fn format_error(error: &bazel_protos::google::rpc::Status) -> String { @@ -706,11 +735,9 @@ mod tests { use super::super::CommandRunner as CommandRunnerTrait; use super::{ - BazelProcessExecutionRequest, BazelProtosProcessExecutionCodec, - BazelProtosProcessExecutionCodecV2, CacheableExecuteProcessRequest, CommandRunner, + ActionSerializer, BazelProcessExecutionRequest, CacheableExecuteProcessRequest, CommandRunner, ExecuteProcessRequest, ExecutionError, ExecutionHistory, FallibleExecuteProcessResult, }; - use crate::cached_execution::SerializableProcessExecutionCodec; use mock::execution_server::MockOperation; use std::collections::{BTreeMap, BTreeSet}; use std::iter::{self, FromIterator}; @@ -754,56 +781,49 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); - want_command.mut_arguments().push("/bin/echo".to_owned()); - want_command.mut_arguments().push("yo".to_owned()); - want_command.mut_environment_variables().push({ - let mut env = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env.set_name("SOME".to_owned()); - env.set_value("value".to_owned()); - env - }); - want_command - .mut_output_files() - .push("other/file".to_owned()); - want_command - .mut_output_files() - .push("path/to/file".to_owned()); - want_command - .mut_output_directories() - .push("directory/name".to_owned()); - - let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); - want_action.set_command_digest( - (&Digest( + let want_command = bazel_protos::build::bazel::remote::execution::v2::Command { + arguments: owned_string_vec(&["/bin/echo", "yo"]), + environment_variables: vec![ + bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: "SOME".to_owned(), + value: "value".to_owned(), + }, + ], + output_files: owned_string_vec(&["other/file", "path/to/file"]), + output_directories: owned_string_vec(&["directory/name"]), + ..Default::default() + }; + + let want_action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(ActionSerializer::convert_digest(&Digest( Fingerprint::from_hex_string( "cc4ddd3085aaffbe0abce22f53b30edbb59896bb4a4f0d76219e48070cd0afe1", ) .unwrap(), 72, - )) - .into(), - ); - want_action.set_input_root_digest((&input_directory.digest()).into()); + ))), + input_root_digest: Some(ActionSerializer::convert_digest(&input_directory.digest())), + ..Default::default() + }; let want_execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some( - (&Digest( - Fingerprint::from_hex_string( - "844c929423444f3392e0dcc89ebf1febbfdf3a2e2fcab7567cc474705a5385e4", - ) - .unwrap(), - 140, - )) - .into(), - ), + action_digest: Some(ActionSerializer::convert_digest(&Digest( + Fingerprint::from_hex_string( + "844c929423444f3392e0dcc89ebf1febbfdf3a2e2fcab7567cc474705a5385e4", + ) + .unwrap(), + 140, + ))), ..Default::default() }; assert_eq!( make_execute_request(&req, &None, &None), - Ok((want_action, want_command, want_execute_request)) + Ok(BazelProcessExecutionRequest::new( + want_action, + want_command, + want_execute_request + )), ); } @@ -830,70 +850,50 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); - want_command.mut_arguments().push("/bin/echo".to_owned()); - want_command.mut_arguments().push("yo".to_owned()); - want_command.mut_environment_variables().push({ - let mut env = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env.set_name("SOME".to_owned()); - env.set_value("value".to_owned()); - env - }); - want_command - .mut_output_files() - .push("other/file".to_owned()); - want_command - .mut_output_files() - .push("path/to/file".to_owned()); - want_command - .mut_output_directories() - .push("directory/name".to_owned()); - - let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); - want_action.set_command_digest( - (&Digest( + let want_command = bazel_protos::build::bazel::remote::execution::v2::Command { + arguments: owned_string_vec(&["/bin/echo", "yo"]), + environment_variables: vec![ + bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: "SOME".to_owned(), + value: "value".to_owned(), + }, + ], + output_files: owned_string_vec(&["other/file", "path/to/file"]), + output_directories: owned_string_vec(&["directory/name"]), + ..Default::default() + }; + + let want_action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(ActionSerializer::convert_digest(&Digest( Fingerprint::from_hex_string( "cc4ddd3085aaffbe0abce22f53b30edbb59896bb4a4f0d76219e48070cd0afe1", ) .unwrap(), 72, - )) - .into(), - ); - want_action.set_input_root_digest((&input_directory.digest()).into()); + ))), + input_root_digest: Some(ActionSerializer::convert_digest(&input_directory.digest())), + ..Default::default() + }; - let mut want_execute_request = bazel_protos::remote_execution::ExecuteRequest::new(); - want_execute_request.set_instance_name("dark-tower".to_owned()); - want_execute_request.set_action_digest( - (&Digest( + let want_execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { + action_digest: Some(ActionSerializer::convert_digest(&Digest( Fingerprint::from_hex_string( "844c929423444f3392e0dcc89ebf1febbfdf3a2e2fcab7567cc474705a5385e4", ) .unwrap(), 140, - )) - .into(), - ); - - let want_execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some( - (&Digest( - Fingerprint::from_hex_string( - "844c929423444f3392e0dcc89ebf1febbfdf3a2e2fcab7567cc474705a5385e4", - ) - .unwrap(), - 140, - )) - .into(), - ), + ))), instance_name: "dark-tower".to_owned(), ..Default::default() }; assert_eq!( make_execute_request(&req, &Some("dark-tower".to_owned()), &None), - Ok((want_action, want_command, want_execute_request)) + Ok(BazelProcessExecutionRequest::new( + want_action, + want_command, + want_execute_request + )), ); } @@ -920,63 +920,53 @@ mod tests { jdk_home: None, }; - let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); - want_command.mut_arguments().push("/bin/echo".to_owned()); - want_command.mut_arguments().push("yo".to_owned()); - want_command.mut_environment_variables().push({ - let mut env = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env.set_name("SOME".to_owned()); - env.set_value("value".to_owned()); - env - }); - want_command.mut_environment_variables().push({ - let mut env = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env.set_name(crate::cached_execution::CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_owned()); - env.set_value("meep".to_owned()); - env - }); - want_command - .mut_output_files() - .push("other/file".to_owned()); - want_command - .mut_output_files() - .push("path/to/file".to_owned()); - want_command - .mut_output_directories() - .push("directory/name".to_owned()); - - let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); - want_action.set_command_digest( - (&Digest( + let want_command = bazel_protos::build::bazel::remote::execution::v2::Command { + arguments: owned_string_vec(&["/bin/echo", "yo"]), + environment_variables: vec![ + bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: "SOME".to_owned(), + value: "value".to_owned(), + }, + bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: crate::cached_execution::CACHE_KEY_GEN_VERSION_ENV_VAR_NAME.to_owned(), + value: "meep".to_owned(), + }, + ], + output_files: owned_string_vec(&["other/file", "path/to/file"]), + output_directories: owned_string_vec(&["directory/name"]), + ..Default::default() + }; + + let want_action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(ActionSerializer::convert_digest(&Digest( Fingerprint::from_hex_string( "1a95e3482dd235593df73dc12b808ec7d922733a40d97d8233c1a32c8610a56d", ) .unwrap(), 109, - )) - .into(), - ); - want_action.set_input_root_digest((&input_directory.digest()).into()); + ))), + input_root_digest: Some(ActionSerializer::convert_digest(&input_directory.digest())), + ..Default::default() + }; let want_execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some( - (&Digest( - Fingerprint::from_hex_string( - "0ee5d4c8ac12513a87c8d949c6883ac533a264d30215126af71a9028c4ab6edf", - ) - .unwrap(), - 140, - )) - .into(), - ), + action_digest: Some(ActionSerializer::convert_digest(&Digest( + Fingerprint::from_hex_string( + "0ee5d4c8ac12513a87c8d949c6883ac533a264d30215126af71a9028c4ab6edf", + ) + .unwrap(), + 140, + ))), ..Default::default() }; assert_eq!( make_execute_request(&req, &None, &Some("meep".to_owned())), - Ok((want_action, want_command, want_execute_request)) + Ok(BazelProcessExecutionRequest::new( + want_action, + want_command, + want_execute_request + )), ); } @@ -994,46 +984,51 @@ mod tests { jdk_home: Some(PathBuf::from("/tmp")), }; - let mut want_command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); - want_command.mut_arguments().push("/bin/echo".to_owned()); - want_command.mut_arguments().push("yo".to_owned()); - want_command.mut_platform().mut_properties().push({ - let mut property = bazel_protos::remote_execution::Platform_Property::new(); - property.set_name("JDK_SYMLINK".to_owned()); - property.set_value(".jdk".to_owned()); - property - }); + let want_command = bazel_protos::build::bazel::remote::execution::v2::Command { + arguments: owned_string_vec(&["/bin/echo", "yo"]), + platform: Some( + bazel_protos::build::bazel::remote::execution::v2::Platform { + properties: vec![ + bazel_protos::build::bazel::remote::execution::v2::platform::Property { + name: "JDK_SYMLINK".to_owned(), + value: ".jdk".to_owned(), + }, + ], + }, + ), + ..Default::default() + }; - let mut want_action = bazel_protos::build::bazel::remote::execution::v2::Action::new(); - want_action.set_command_digest( - (&Digest( + let want_action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(ActionSerializer::convert_digest(&Digest( Fingerprint::from_hex_string( "f373f421b328ddeedfba63542845c0423d7730f428dd8e916ec6a38243c98448", ) .unwrap(), 38, - )) - .into(), - ); - want_action.set_input_root_digest((&input_directory.digest()).into()); + ))), + input_root_digest: Some(ActionSerializer::convert_digest(&input_directory.digest())), + ..Default::default() + }; let want_execute_request = bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest { - action_digest: Some( - (&Digest( - Fingerprint::from_hex_string( - "b1fb7179ce496995a4e3636544ec000dca1b951f1f6216493f6c7608dc4dd910", - ) - .unwrap(), - 140, - )) - .into(), - ), + action_digest: Some(ActionSerializer::convert_digest(&Digest( + Fingerprint::from_hex_string( + "b1fb7179ce496995a4e3636544ec000dca1b951f1f6216493f6c7608dc4dd910", + ) + .unwrap(), + 140, + ))), ..Default::default() }; assert_eq!( make_execute_request(&req, &None, &None), - Ok((want_action, want_command, want_execute_request)) + Ok(BazelProcessExecutionRequest::new( + want_action, + want_command, + want_execute_request + )), ); } @@ -1059,7 +1054,7 @@ mod tests { &None, ) .unwrap() - .2, + .execute_request, vec![], )) }; @@ -1082,7 +1077,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_successful_operation( @@ -1179,7 +1174,7 @@ mod tests { op_name.clone(), make_execute_request(&echo_roland_request(), &None, &None) .unwrap() - .2, + .execute_request, vec![make_successful_operation( &op_name.clone(), StdoutType::Raw(test_stdout.string()), @@ -1268,7 +1263,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, Vec::from_iter( iter::repeat(make_incomplete_operation(&op_name)) .take(4) @@ -1319,7 +1314,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_delayed_incomplete_operation(&op_name, delayed_operation_time), @@ -1344,7 +1339,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_canceled_operation(Some(Duration::from_millis(100))), @@ -1383,7 +1378,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), MockOperation::new({ @@ -1417,7 +1412,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![MockOperation::new( bazel_protos::google::longrunning::Operation { name: op_name.clone(), @@ -1451,7 +1446,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), MockOperation::new(bazel_protos::google::longrunning::Operation { @@ -1486,7 +1481,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![MockOperation::new( bazel_protos::google::longrunning::Operation { name: op_name.clone(), @@ -1514,7 +1509,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), MockOperation::new(bazel_protos::google::longrunning::Operation { @@ -1543,7 +1538,7 @@ mod tests { op_name.clone(), make_execute_request(&cat_roland_request(), &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_precondition_failure_operation(vec![missing_preconditionfailure_violation( @@ -1636,7 +1631,7 @@ mod tests { op_name.clone(), make_execute_request(&cat_roland_request(), &None, &None) .unwrap() - .2, + .execute_request, vec![ //make_incomplete_operation(&op_name), MockOperation { @@ -1718,7 +1713,7 @@ mod tests { op_name.clone(), make_execute_request(&cat_roland_request(), &None, &None) .unwrap() - .2, + .execute_request, // We won't get as far as trying to run the operation, so don't expect any requests whose // responses we would need to stub. vec![], @@ -1961,23 +1956,24 @@ mod tests { #[test] fn digest_command() { - let mut command = bazel_protos::build::bazel::remote::execution::v2::Command::new(); - command.mut_arguments().push("/bin/echo".to_string()); - command.mut_arguments().push("foo".to_string()); + let env1 = bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: "A".to_string(), + value: "a".to_string(), + }; - let mut env1 = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env1.set_name("A".to_string()); - env1.set_value("a".to_string()); - command.mut_environment_variables().push(env1); + let env2 = bazel_protos::build::bazel::remote::execution::v2::command::EnvironmentVariable { + name: "B".to_string(), + value: "b".to_string(), + }; - let mut env2 = - bazel_protos::build::bazel::remote::execution::v2::Command_EnvironmentVariable::new(); - env2.set_name("B".to_string()); - env2.set_value("b".to_string()); - command.mut_environment_variables().push(env2); + let command = bazel_protos::build::bazel::remote::execution::v2::Command { + arguments: owned_string_vec(&["/bin/echo", "foo"]), + environment_variables: vec![env1, env2], + ..Default::default() + }; - let digest = ActionSerializer::digest_proto(&command).unwrap(); + let digest = + fs::Store::digest_bytes(&ActionSerializer::encode_command_proto(&command).unwrap()); assert_eq!( &digest.0.to_hex(), @@ -1997,7 +1993,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_successful_operation( @@ -2035,7 +2031,7 @@ mod tests { op_name.clone(), make_execute_request(&execute_request, &None, &None) .unwrap() - .2, + .execute_request, vec![ make_incomplete_operation(&op_name), make_incomplete_operation(&op_name), @@ -2405,10 +2401,10 @@ mod tests { .build(); let (store, _) = create_command_runner_with_store("127.0.0.1:0".to_owned(), &cas, None); - let codec = codec_from_store(store, None); + let action_serializer = ActionSerializer::new(store); let mut runtime = tokio::runtime::Runtime::new().unwrap(); - let result = runtime.block_on(codec.extract_action_result(result.clone())); + let result = runtime.block_on(action_serializer.extract_action_result(&result)); runtime.shutdown_now().wait().unwrap(); result .map(|res| res.output_directory) @@ -2478,57 +2474,26 @@ mod tests { } } - fn codec_from_store( - store: fs::Store, - instance_name: Option, - ) -> BazelProtosProcessExecutionCodecV2 { - BazelProtosProcessExecutionCodecV2::new(store, instance_name) - } - - fn codec(dir: PathBuf, instance_name: Option) -> BazelProtosProcessExecutionCodecV2 { - let pool = Arc::new(fs::ResettablePool::new("test-pool-".to_owned())); - let store = fs::Store::local_only(dir, pool.clone()).unwrap(); - codec_from_store(store, instance_name) - } - - fn codec_convert_execute_request( - dir: PathBuf, + fn serializer_convert_execute_request( req: ExecuteProcessRequest, instance_name: Option, cache_key_gen_version: Option, ) -> Result { - let codec = codec(dir, instance_name); - codec.convert_request_to_action(CacheableExecuteProcessRequest::new( - req, - cache_key_gen_version, - )) + BazelProcessExecutionRequest::convert_execute_request( + &CacheableExecuteProcessRequest::new(req, cache_key_gen_version), + &instance_name, + ) } fn make_execute_request( req: &ExecuteProcessRequest, instance_name: &Option, cache_key_gen_version: &Option, - ) -> Result< - ( - bazel_protos::build::bazel::remote::execution::v2::Action, - bazel_protos::build::bazel::remote::execution::v2::Command, - bazel_protos::build::bazel::remote::execution::v2::ExecuteRequest, - ), - String, - > { - let store_dir = TempDir::new().unwrap(); - codec_convert_execute_request( - store_dir.path().to_path_buf(), + ) -> Result { + serializer_convert_execute_request( req.clone(), instance_name.clone(), cache_key_gen_version.clone(), ) - .map( - |BazelProcessExecutionRequest { - action, - command, - execute_request, - }| { (action, command, execute_request) }, - ) } } From 5b526b659555fc0d576dbcc41242c185d3e4c6d9 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 21:21:25 -0800 Subject: [PATCH 06/10] invert the wrapping of our single output directory in bazel's ActionResult --- .../process_execution/src/cached_execution.rs | 206 ++++++++++++++++-- .../engine/process_execution/src/local.rs | 19 +- .../engine/process_execution/src/remote.rs | 7 +- 3 files changed, 209 insertions(+), 23 deletions(-) diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index b27cea3e784..ed058551ee8 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -40,7 +40,7 @@ use futures::future::{self, Future}; use log::debug; use fs::{File, PathStat}; -use hashing::Digest; +use hashing::{Digest, Fingerprint}; use super::{CommandRunner, ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult}; @@ -90,6 +90,12 @@ impl CacheableExecuteProcessResult { } } +/// ??? +pub enum OutputDirWrapping { + Direct, + TopLevelWrapped, +} + /// ???/it's called "immediate" because it's a best-effort thing located locally (???) pub trait ImmediateExecutionCache: Send + Sync { fn record_process_result( @@ -112,6 +118,13 @@ impl ActionSerializer { ActionSerializer { store } } + fn extract_digest( + digest: &bazel_protos::build::bazel::remote::execution::v2::Digest, + ) -> Result { + let fingerprint = Fingerprint::from_hex_string(&digest.hash)?; + Ok(Digest(fingerprint, digest.size_bytes as usize)) + } + pub fn convert_digest( digest: &Digest, ) -> bazel_protos::build::bazel::remote::execution::v2::Digest { @@ -315,10 +328,43 @@ impl ActionSerializer { } } + fn extract_output_files_with_single_containing_directory( + result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, + ) -> Result { + let invalid_containing_dir = || { + format!("Error: invalid output directory for result {:?}. An process execution extracted as an ActionResult from the process execution cache must will always contain a single top-level directory with the path \"\".", + result) + }; + + if result.output_files.is_empty() && result.output_directories.len() == 1 { + // A single directory (with a provided digest), which should be at the path "". + let dir = result.output_directories.last().unwrap(); + if dir.path.is_empty() { + let proto_digest = dir.tree_digest.clone().unwrap(); + Self::extract_digest(&proto_digest) + } else { + Err(invalid_containing_dir()) + } + } else { + Err(invalid_containing_dir()) + } + } + fn extract_output_files( &self, result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, + wrapping: OutputDirWrapping, ) -> BoxFuture { + match wrapping { + OutputDirWrapping::Direct => (), + OutputDirWrapping::TopLevelWrapped => { + return future::result(Self::extract_output_files_with_single_containing_directory( + result, + )) + .to_boxed(); + } + } + // Get Digests of output Directories. // Then we'll make a Directory for the output files, and merge them. let output_directories = result.output_directories.clone(); @@ -447,12 +493,13 @@ impl ActionSerializer { pub fn extract_action_result( &self, res: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, + wrapping: OutputDirWrapping, ) -> BoxFuture { let exit_code = res.exit_code; self .extract_stdout(&res) .join(self.extract_stderr(&res)) - .join(self.extract_output_files(&res)) + .join(self.extract_output_files(&res, wrapping)) .map( move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { stdout, @@ -505,7 +552,11 @@ impl ImmediateExecutionCache cache - .extract_action_result(&action_result) + // NB: FallibleExecuteProcessResult always wraps everything in a *single* output + // directory, which is then converted into an OutputDirectory for the ActionResult proto + // at the path "" in convert_result_to_action_result(), so we have to pull the contents + // out of that single dir with the path "". + .extract_action_result(&action_result, OutputDirWrapping::TopLevelWrapped) .map(Some) .to_boxed(), None => future::result(Ok(None)).to_boxed(), @@ -602,31 +653,105 @@ impl CommandRunner for CachingCommandRunner { mod tests { use super::{ ActionSerializer, CacheableExecuteProcessRequest, CacheableExecuteProcessResult, - CachingCommandRunner, CommandRunner, ExecuteProcessRequest, ImmediateExecutionCache, + CachingCommandRunner, CommandRunner, ExecuteProcessRequest, FallibleExecuteProcessResult, + ImmediateExecutionCache, }; + use crate::local::testutils::find_bash; use futures::future::Future; + use hashing::{Digest, Fingerprint}; use std::collections::{BTreeMap, BTreeSet}; use std::ops::Deref; use std::path::Path; + use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; use tempfile::TempDir; + use testutil::data::{TestData, TestDirectory}; use testutil::owned_string_vec; - // // TODO: test codec back and forth - // #[test] - // fn bazel_proto_process_execution_codec() { - // let req = ExecuteProcessRequest { - // argv: owned_string_vec(&["ls", "-R", "/"]), - // env: BTreeMap::new(), - // input_files: fs::EMPTY_DIGEST, - // output_files: - // } - // } + #[test] + fn encode_action() { + let input_directory = TestDirectory::containing_roland(); + let req = ExecuteProcessRequest { + argv: owned_string_vec(&["/bin/echo", "yo"]), + env: vec![("SOME".to_owned(), "value".to_owned())] + .into_iter() + .collect(), + input_files: input_directory.digest(), + // Intentionally poorly sorted: + output_files: vec!["path/to/file", "other/file"] + .into_iter() + .map(PathBuf::from) + .collect(), + output_directories: vec!["directory/name"] + .into_iter() + .map(PathBuf::from) + .collect(), + timeout: Duration::from_millis(1000), + description: "some description".to_owned(), + jdk_home: None, + }; + + let want_action = bazel_protos::build::bazel::remote::execution::v2::Action { + command_digest: Some(ActionSerializer::convert_digest(&Digest( + Fingerprint::from_hex_string( + "cc4ddd3085aaffbe0abce22f53b30edbb59896bb4a4f0d76219e48070cd0afe1", + ) + .unwrap(), + 72, + ))), + input_root_digest: Some(ActionSerializer::convert_digest(&input_directory.digest())), + ..Default::default() + }; + + assert_eq!( + Ok(want_action), + ActionSerializer::convert_request_to_action(&CacheableExecuteProcessRequest::new(req, None)) + ); + } + + #[test] + fn encode_empty_action_result() { + let testdata_empty = TestData::empty(); + + let empty_proto_digest = bazel_protos::build::bazel::remote::execution::v2::Digest { + hash: fs::EMPTY_DIGEST.0.to_hex(), + size_bytes: fs::EMPTY_DIGEST.1 as i64, + }; + + let want_action_result = bazel_protos::build::bazel::remote::execution::v2::ActionResult { + output_files: vec![], + output_directories: vec![ + bazel_protos::build::bazel::remote::execution::v2::OutputDirectory { + path: "".to_string(), + tree_digest: Some(empty_proto_digest.clone()), + }, + ], + exit_code: 0, + stdout_raw: vec![], + stdout_digest: Some(empty_proto_digest.clone()), + stderr_raw: vec![], + stderr_digest: Some(empty_proto_digest.clone()), + execution_metadata: None, + }; + + let empty_result = FallibleExecuteProcessResult { + stdout: testdata_empty.bytes(), + stderr: testdata_empty.bytes(), + exit_code: 0, + output_directory: fs::EMPTY_DIGEST, + execution_attempts: vec![], + }; + + assert_eq!( + want_action_result, + ActionSerializer::convert_result_to_action_result(&empty_result.into_cacheable()) + ); + } #[test] #[cfg(unix)] - fn cached_process_execution() { + fn cached_process_execution_stdout() { let random_perl = output_only_process_request(owned_string_vec(&[ "/usr/bin/perl", "-e", @@ -647,6 +772,9 @@ mod tests { ); let caching_runner = make_caching_runner(base_runner.clone(), action_serializer.clone(), None); let process_result = caching_runner.run(random_perl.clone()).wait().unwrap(); + // The process run again without caching is different. + let base_process_result = base_runner.run(random_perl.clone()).wait().unwrap(); + assert!(base_process_result != process_result); assert_eq!(0, process_result.exit_code); // A "cacheable" process execution result won't have e.g. the number of attempts that the // process was tried, for idempotency, but everything else should be the same. @@ -746,6 +874,54 @@ mod tests { assert!(second_string_perl_number != new_perl_number); } + #[test] + #[cfg(unix)] + fn cached_process_execution_output_files() { + let make_file = ExecuteProcessRequest { + argv: vec![ + find_bash(), + "-c".to_owned(), + "/usr/bin/perl -e 'print(rand(10))' > wow.txt".to_string(), + ], + env: BTreeMap::new(), + input_files: fs::EMPTY_DIGEST, + output_files: vec!["wow.txt"].into_iter().map(PathBuf::from).collect(), + output_directories: BTreeSet::new(), + timeout: Duration::from_millis(1000), + description: "make a nondeterministic file".to_string(), + jdk_home: None, + }; + let store_dir = TempDir::new().unwrap(); + let work_dir = TempDir::new().unwrap(); + let (_, base_runner, action_serializer) = cache_in_dir(store_dir.path(), work_dir.path()); + let cacheable_make_file = CacheableExecuteProcessRequest { + req: make_file.clone(), + cache_key_gen_version: None, + }; + assert_eq!( + Ok(None), + action_serializer + .load_process_result(&cacheable_make_file) + .wait() + ); + let caching_runner = make_caching_runner(base_runner.clone(), action_serializer.clone(), None); + let base_process_result = base_runner.run(make_file.clone()).wait().unwrap(); + let process_result = caching_runner.run(make_file.clone()).wait().unwrap(); + // The process run again without caching is different. + assert!(base_process_result != process_result); + assert_eq!(0, process_result.exit_code); + assert_eq!( + process_result.clone().into_cacheable(), + action_serializer + .load_process_result(&cacheable_make_file) + .wait() + .unwrap() + .unwrap() + ); + let second_process_result = caching_runner.run(make_file.clone()).wait().unwrap(); + assert_eq!(second_process_result, process_result); + } + fn output_only_process_request(argv: Vec) -> ExecuteProcessRequest { ExecuteProcessRequest { argv, diff --git a/src/rust/engine/process_execution/src/local.rs b/src/rust/engine/process_execution/src/local.rs index 5e24d85aecd..57bf0cf00ea 100644 --- a/src/rust/engine/process_execution/src/local.rs +++ b/src/rust/engine/process_execution/src/local.rs @@ -293,6 +293,7 @@ impl super::CommandRunner for CommandRunner { }) .map(Arc::new) .and_then(|posix_fs| { + eprintln!("output_file_paths: {:?}", output_file_paths); CommandRunner::construct_output_snapshot( store, posix_fs, @@ -337,13 +338,12 @@ mod tests { use super::super::CommandRunner as CommandRunnerTrait; use super::{ExecuteProcessRequest, FallibleExecuteProcessResult}; + use crate::local::testutils::*; use fs; use futures::Future; use std; use std::collections::{BTreeMap, BTreeSet}; - use std::env; - use std::os::unix::fs::PermissionsExt; - use std::path::{Path, PathBuf}; + use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; use tempfile::TempDir; @@ -883,8 +883,15 @@ mod tests { }; runner.run(req).wait() } +} + +#[cfg(test)] +pub mod testutils { + use std::env; + use std::os::unix::fs::PermissionsExt; + use std::path::{Path, PathBuf}; - fn find_bash() -> String { + pub fn find_bash() -> String { which("bash") .expect("No bash on PATH") .to_str() @@ -892,7 +899,7 @@ mod tests { .to_owned() } - fn which(executable: &str) -> Option { + pub fn which(executable: &str) -> Option { if let Some(paths) = env::var_os("PATH") { for path in env::split_paths(&paths) { let executable_path = path.join(executable); @@ -904,7 +911,7 @@ mod tests { None } - fn is_executable(path: &Path) -> bool { + pub fn is_executable(path: &Path) -> bool { std::fs::metadata(path) .map(|meta| meta.permissions().mode() & 0o100 == 0o100) .unwrap_or(false) diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 0b6199fc68f..57f4c5b0598 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -16,6 +16,7 @@ use super::{ ActionSerializer, CacheableExecuteProcessRequest, ExecuteProcessRequest, ExecutionStats, FallibleExecuteProcessResult, }; +use crate::cached_execution::OutputDirWrapping; use std; use std::cmp::min; @@ -390,7 +391,7 @@ impl CommandRunner { if let Some(result) = maybe_result { return self .action_serializer - .extract_action_result(&result) + .extract_action_result(&result, OutputDirWrapping::Direct) .map(|cacheable_result| cacheable_result.with_execution_attempts(execution_attempts)) .map_err(move |err| { ExecutionError::Fatal(format!( @@ -738,6 +739,7 @@ mod tests { ActionSerializer, BazelProcessExecutionRequest, CacheableExecuteProcessRequest, CommandRunner, ExecuteProcessRequest, ExecutionError, ExecutionHistory, FallibleExecuteProcessResult, }; + use crate::cached_execution::OutputDirWrapping; use mock::execution_server::MockOperation; use std::collections::{BTreeMap, BTreeSet}; use std::iter::{self, FromIterator}; @@ -2404,7 +2406,8 @@ mod tests { let action_serializer = ActionSerializer::new(store); let mut runtime = tokio::runtime::Runtime::new().unwrap(); - let result = runtime.block_on(action_serializer.extract_action_result(&result)); + let result = + runtime.block_on(action_serializer.extract_action_result(&result, OutputDirWrapping::Direct)); runtime.shutdown_now().wait().unwrap(); result .map(|res| res.output_directory) From be4f53c77c20c431dffc9fd812e95757c27e9be5 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 21:30:28 -0800 Subject: [PATCH 07/10] add note --- src/rust/engine/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 058d2db966d..1100db19e28 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -198,6 +198,7 @@ pub extern "C" fn scheduler_create( build_root_buf: Buffer, work_dir_buf: Buffer, local_store_dir_buf: Buffer, + // TODO: need some testing modifying this variable! local_execution_process_cache_namespace: Buffer, ignore_patterns_buf: BufferBuffer, root_type_ids: TypeIdBuffer, From c6ef41cfdc54e92e5d3bc5a77719b6adc981150a Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 22:02:52 -0800 Subject: [PATCH 08/10] make creating a DbEnv a little nicer --- src/rust/engine/fs/src/store.rs | 61 ++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/rust/engine/fs/src/store.rs b/src/rust/engine/fs/src/store.rs index 898b6c995cd..2a383df44e6 100644 --- a/src/rust/engine/fs/src/store.rs +++ b/src/rust/engine/fs/src/store.rs @@ -789,9 +789,37 @@ mod local { inner: Arc, } + /// + /// A simple wrapper for an lmdb database with an associated environment. + /// struct DbEnv(Arc, Arc); impl DbEnv { + fn new(path: &Path, db_name: &str, flags: DatabaseFlags) -> Result, String> { + super::super::safe_create_dir_all(&path) + .map_err(|err| format!("Error making directory for store at {:?}: {:?}", path, err))?; + let process_executions_env = Environment::new() + .set_max_dbs(1) + .set_map_size(MAX_LOCAL_STORE_SIZE_BYTES) + .open(&path) + .map_err(|err| { + format!( + "Error making process execution Environment for db at {:?}: {:?}", + path, err + ) + })?; + process_executions_env + .create_db(Some(db_name), flags) + .map_err(|e| { + format!( + "Error creating/opening database named {:?} at {:?}: {}", + db_name, path, e + ) + }) + .map(|db| Arc::new(DbEnv(Arc::new(db), Arc::new(process_executions_env)))) + } + + // Arc requires calling a method to extract the constituent fields. fn get(&self) -> (Arc, Arc) { (Arc::clone(&self.0), Arc::clone(&self.1)) } @@ -814,38 +842,17 @@ mod local { let files_root = root.join("files"); let directories_root = root.join("directories"); let process_executions_root = root.join("process_executions"); - super::super::safe_create_dir_all(&process_executions_root).map_err(|err| { - format!( - "Error making directory for process execution store at {:?}: {:?}", - process_executions_root, err - ) - })?; - let process_executions_env = Environment::new() - .set_max_dbs(1) - .set_map_size(MAX_LOCAL_STORE_SIZE_BYTES) - // TODO: does the Environment need to be opened in a subdirectory of the db dir? see - // ShardedLmdb::make_env()! - .open(&process_executions_root) - .map_err(|err| { - format!( - "Error making process execution Environment for db at {:?}: {:?}", - process_executions_root, err - ) - })?; + Ok(ByteStore { inner: Arc::new(InnerStore { pool: pool, file_dbs: ShardedLmdb::new(files_root.clone()).map(Arc::new), directory_dbs: ShardedLmdb::new(directories_root.clone()).map(Arc::new), - process_execution_db: process_executions_env - .create_db(Some("process_executions_content"), DatabaseFlags::empty()) - .map_err(|e| { - format!( - "Error creating/opening process execution content database at {:?}: {}", - process_executions_root, e - ) - }) - .map(|db| Arc::new(DbEnv(Arc::new(db), Arc::new(process_executions_env)))), + process_execution_db: DbEnv::new( + &process_executions_root, + "process_executions_content", + DatabaseFlags::empty(), + ), }), }) } From c66a528fd64852bae748ce4d905bdf52e67bfe31 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 22:20:52 -0800 Subject: [PATCH 09/10] make the OutputDirWrapping conditional less awkward --- .../process_execution/src/cached_execution.rs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/rust/engine/process_execution/src/cached_execution.rs b/src/rust/engine/process_execution/src/cached_execution.rs index ed058551ee8..8c48c733226 100644 --- a/src/rust/engine/process_execution/src/cached_execution.rs +++ b/src/rust/engine/process_execution/src/cached_execution.rs @@ -353,18 +353,7 @@ impl ActionSerializer { fn extract_output_files( &self, result: &bazel_protos::build::bazel::remote::execution::v2::ActionResult, - wrapping: OutputDirWrapping, ) -> BoxFuture { - match wrapping { - OutputDirWrapping::Direct => (), - OutputDirWrapping::TopLevelWrapped => { - return future::result(Self::extract_output_files_with_single_containing_directory( - result, - )) - .to_boxed(); - } - } - // Get Digests of output Directories. // Then we'll make a Directory for the output files, and merge them. let output_directories = result.output_directories.clone(); @@ -496,10 +485,17 @@ impl ActionSerializer { wrapping: OutputDirWrapping, ) -> BoxFuture { let exit_code = res.exit_code; + let extracted_output_files = match wrapping { + OutputDirWrapping::Direct => self.extract_output_files(&res), + OutputDirWrapping::TopLevelWrapped => future::result( + Self::extract_output_files_with_single_containing_directory(&res), + ) + .to_boxed(), + }; self .extract_stdout(&res) .join(self.extract_stderr(&res)) - .join(self.extract_output_files(&res, wrapping)) + .join(extracted_output_files) .map( move |((stdout, stderr), output_directory)| CacheableExecuteProcessResult { stdout, From d9e577ac393d604fe251d859b61232c64dfb880d Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sun, 27 Jan 2019 22:57:53 -0800 Subject: [PATCH 10/10] remove without_execution_attempts() test method --- src/rust/engine/process_execution/src/lib.rs | 9 ----- .../engine/process_execution/src/remote.rs | 34 +++++++++---------- 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 8022879a2eb..47a7af7794b 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -118,15 +118,6 @@ impl FallibleExecuteProcessResult { } } -// TODO: remove this method! -#[cfg(test)] -impl FallibleExecuteProcessResult { - pub fn without_execution_attempts(mut self) -> Self { - self.execution_attempts = vec![]; - self - } -} - #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub struct ExecutionStats { uploaded_bytes: usize, diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 57f4c5b0598..78a9c1cb245 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -1095,13 +1095,12 @@ mod tests { let result = run_command_remote(mock_server.address(), execute_request).unwrap(); assert_eq!( - result.without_execution_attempts(), - FallibleExecuteProcessResult { + result.into_cacheable(), + CacheableExecuteProcessResult { stdout: as_bytes("foo"), stderr: as_bytes(""), exit_code: 0, output_directory: fs::EMPTY_DIGEST, - execution_attempts: vec![], } ); } @@ -1124,8 +1123,8 @@ mod tests { .unwrap() ) .unwrap() - .without_execution_attempts(), - FallibleExecuteProcessResult { + .into_cacheable(), + CacheableExecuteProcessResult { stdout: testdata.bytes(), stderr: testdata_empty.bytes(), exit_code: 0, @@ -1153,8 +1152,8 @@ mod tests { .unwrap() ) .unwrap() - .without_execution_attempts(), - FallibleExecuteProcessResult { + .into_cacheable(), + CacheableExecuteProcessResult { stdout: testdata_empty.bytes(), stderr: testdata.bytes(), exit_code: 0, @@ -1221,8 +1220,8 @@ mod tests { let result = rt.block_on(cmd_runner.run(echo_roland_request())).unwrap(); rt.shutdown_now().wait().unwrap(); assert_eq!( - result.without_execution_attempts(), - FallibleExecuteProcessResult { + result.into_cacheable(), + CacheableExecuteProcessResult { stdout: test_stdout.bytes(), stderr: test_stderr.bytes(), exit_code: 0, @@ -1282,8 +1281,8 @@ mod tests { let result = run_command_remote(mock_server.address(), execute_request).unwrap(); assert_eq!( - result.without_execution_attempts(), - FallibleExecuteProcessResult { + result.into_cacheable(), + CacheableExecuteProcessResult { stdout: as_bytes("foo"), stderr: as_bytes(""), exit_code: 0, @@ -1358,8 +1357,8 @@ mod tests { let result = run_command_remote(mock_server.address(), execute_request).unwrap(); assert_eq!( - result.without_execution_attempts(), - FallibleExecuteProcessResult { + result.into_cacheable(), + CacheableExecuteProcessResult { stdout: as_bytes("foo"), stderr: as_bytes(""), exit_code: 0, @@ -1601,8 +1600,8 @@ mod tests { ); rt.shutdown_now().wait().unwrap(); assert_eq!( - result.unwrap().without_execution_attempts(), - FallibleExecuteProcessResult { + result.unwrap().into_cacheable(), + CacheableExecuteProcessResult { stdout: roland.bytes(), stderr: Bytes::from(""), exit_code: 0, @@ -1790,12 +1789,11 @@ mod tests { #[test] fn extract_execute_response_success() { - let want_result = FallibleExecuteProcessResult { + let want_result = CacheableExecuteProcessResult { stdout: as_bytes("roland"), stderr: Bytes::from("simba"), exit_code: 17, output_directory: TestDirectory::nested().digest(), - execution_attempts: vec![], }; let response = bazel_protos::build::bazel::remote::execution::v2::ExecuteResponse { @@ -1831,7 +1829,7 @@ mod tests { assert_eq!( extract_execute_response(operation) .unwrap() - .without_execution_attempts(), + .into_cacheable(), want_result ); }