diff --git a/workspaces/Cargo.lock b/workspaces/Cargo.lock index 3e8117b1791..65cae0e8106 100644 --- a/workspaces/Cargo.lock +++ b/workspaces/Cargo.lock @@ -911,11 +911,6 @@ name = "fnv" version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" -[[package]] -name = "fs_extra" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" - [[package]] name = "fuchsia-cprng" version = "0.1.1" @@ -1406,6 +1401,8 @@ name = "migration-helpers" version = "0.1.0" dependencies = [ "apiserver 0.1.0", + "maplit 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)", + "serde_json 1.0.44 (registry+https://github.com/rust-lang/crates.io-index)", "snafu 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", "toml 0.5.5 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -1416,7 +1413,6 @@ version = "0.1.0" dependencies = [ "cargo-readme 3.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "data_store_version 0.1.0", - "fs_extra 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "nix 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3290,7 +3286,6 @@ dependencies = [ "checksum filetime 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)" = "1ff6d4dab0aa0c8e6346d46052e93b13a16cf847b54ed357087c35011048cc7d" "checksum flate2 1.0.13 (registry+https://github.com/rust-lang/crates.io-index)" = "6bd6d6f4752952feb71363cffc9ebac9411b75b87c6ab6058c40c8900cf43c0f" "checksum fnv 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)" = "2fad85553e09a6f881f739c29f0b00b0f01357c743266d478b68951ce23285f3" -"checksum fs_extra 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5f2a4a2034423744d2cc7ca2068453168dcdb82c438419e639a26bd87839c674" "checksum fuchsia-cprng 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a06f77d526c1a601b7c4cdd98f54b5eaabffc14d5f2f0296febdc7f357c6d3ba" "checksum fuchsia-zircon 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "2e9763c69ebaae630ba35f74888db465e49e259ba1bc0eda7d06f4a067615d82" "checksum fuchsia-zircon-sys 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "3dcaa9ae7725d12cdb85b3ad99a434db70b468c09ded17e012d86b5c1010f7a7" diff --git a/workspaces/api/apiserver/src/datastore/error.rs b/workspaces/api/apiserver/src/datastore/error.rs index 124f7b53b95..cd6bb794365 100644 --- a/workspaces/api/apiserver/src/datastore/error.rs +++ b/workspaces/api/apiserver/src/datastore/error.rs @@ -23,6 +23,9 @@ pub enum Error { #[snafu(display("Reading key '{}' failed: {}", key, source))] KeyRead { key: String, source: io::Error }, + #[snafu(display("Removing key at '{}' failed: {}", path.display(), source))] + DeleteKey { path: PathBuf, source: io::Error }, + #[snafu(display("IO error on '{}': {}", path.display(), source))] Io { path: PathBuf, source: io::Error }, diff --git a/workspaces/api/apiserver/src/datastore/filesystem.rs b/workspaces/api/apiserver/src/datastore/filesystem.rs index 37e76d14839..5d886ed0a5d 100644 --- a/workspaces/api/apiserver/src/datastore/filesystem.rs +++ b/workspaces/api/apiserver/src/datastore/filesystem.rs @@ -97,6 +97,72 @@ impl FilesystemDataStore { Ok(path_str.into()) } + + /// Deletes the given path from the filesystem. Also removes the parent directory if empty + /// (repeatedly, up to the base path), so as to have consistent artifacts on the filesystem + /// after adding and removing keys. + /// + /// If the path doesn't exist, we still return Ok for idempotency, but if it exists and we + /// fail to remove it, we return Err. + /// + /// If we fail to remove an empty directory, we log an error, but still return Ok. (The + /// error for trying to remove an empty directory is not specific, and we don't want to rely + /// on platform-specific error codes or the error description. We could check the directory + /// contents ourself, but it would be more complex and subject to timing issues.) + fn delete_key_path

(&mut self, path: P, committed: Committed) -> Result<()> + where + P: AsRef, + { + let path = path.as_ref(); + + // Remove the file. If it doesn't exist, we're still OK. + match fs::remove_file(path) { + Ok(()) => {} + Err(e) => { + if e.kind() != io::ErrorKind::NotFound { + return Err(e).context(error::DeleteKey { path }); + } + } + } + + // Remove the directory if it's empty, i.e. if the setting we removed was the last setting + // in that prefix. Continue up the tree until the base, in case it was the only thing in + // that subtree. + let base = self.base_path(committed); + if let Some(parent) = path.parent() { + // Note: ancestors() includes 'parent' itself + for parent in parent.ancestors() { + // Stop at the base directory; we don't expect anything here or above to be empty, + // but stop as a safeguard. + if parent == base { + break; + } + if let Err(e) = fs::remove_dir(parent) { + // If the directory doesn't exist, continue up the tree. Modulo timing issues, + // this means the key didn't exist either, which means a previous attempt to remove + // the directory failed or we got an unset request for a bogus key. Either way, we + // can clean up and make things consistent. + if e.kind() == io::ErrorKind::NotFound { + continue; + + // "Directory not empty" doesn't have its own ErrorKind, so we have to check a + // platform-specific error number or the error description, neither of which is + // ideal. Still, we can at least log an error in the case we know. Don't + // fail, though, because we've still accomplished our main purpose. + } else if e.raw_os_error() != Some(39) { + error!( + "Failed to delete directory '{}' we believe is empty: {}", + parent.display(), + e + ); + } + // We won't be able to delete parent directories if this one still exists. + break; + } + } + } + Ok(()) + } } // Filesystem helpers @@ -382,6 +448,11 @@ impl DataStore for FilesystemDataStore { write_file_mkdir(path, value) } + fn unset_key(&mut self, key: &Key, committed: Committed) -> Result<()> { + let path = self.data_path(key, committed)?; + self.delete_key_path(path, committed) + } + fn get_metadata_raw(&self, metadata_key: &Key, data_key: &Key) -> Result> { let path = self.metadata_path(metadata_key, data_key, Committed::Live)?; read_file_for_key(&metadata_key, &path) @@ -397,6 +468,11 @@ impl DataStore for FilesystemDataStore { write_file_mkdir(path, value) } + fn unset_metadata(&mut self, metadata_key: &Key, data_key: &Key) -> Result<()> { + let path = self.metadata_path(metadata_key, data_key, Committed::Live)?; + self.delete_key_path(path, Committed::Live) + } + /// We commit by copying pending keys to live, then removing pending. Something smarter (lock, /// atomic flip, etc.) will be required to make the server concurrent. fn commit(&mut self) -> Result> { diff --git a/workspaces/api/apiserver/src/datastore/memory.rs b/workspaces/api/apiserver/src/datastore/memory.rs index 70d3fb9915f..421f156366a 100644 --- a/workspaces/api/apiserver/src/datastore/memory.rs +++ b/workspaces/api/apiserver/src/datastore/memory.rs @@ -9,7 +9,7 @@ use std::mem; use super::{Committed, DataStore, Key, Result}; #[derive(Debug)] -pub(crate) struct MemoryDataStore { +pub struct MemoryDataStore { // Uncommitted (pending) data. pending: HashMap, // Committed (live) data. @@ -20,7 +20,7 @@ pub(crate) struct MemoryDataStore { } impl MemoryDataStore { - pub(crate) fn new() -> Self { + pub fn new() -> Self { Self { pending: HashMap::new(), live: HashMap::new(), @@ -103,6 +103,11 @@ impl DataStore for MemoryDataStore { Ok(()) } + fn unset_key(&mut self, key: &Key, committed: Committed) -> Result<()> { + self.dataset_mut(committed).remove(key); + Ok(()) + } + fn key_populated(&self, key: &Key, committed: Committed) -> Result { Ok(self.dataset(committed).contains_key(key)) } @@ -133,6 +138,14 @@ impl DataStore for MemoryDataStore { Ok(()) } + fn unset_metadata(&mut self, metadata_key: &Key, data_key: &Key) -> Result<()> { + // If we have any metadata for this data key, remove the given metadata key. + if let Some(metadata_for_data) = self.metadata.get_mut(data_key) { + metadata_for_data.remove(metadata_key); + } + Ok(()) + } + fn commit(&mut self) -> Result> { // We need a clone of the pending keys so we can set_keys (which holds &mut self) and we // have to clone the keys anyway for the return value. @@ -164,7 +177,7 @@ mod test { use maplit::hashset; #[test] - fn get_set() { + fn get_set_unset() { let mut m = MemoryDataStore::new(); let k = Key::new(KeyType::Data, "memtest").unwrap(); let v = "memvalue"; @@ -178,6 +191,12 @@ mod test { m.get_metadata_raw(&mdkey, &k).unwrap(), Some(md.to_string()) ); + + m.unset_metadata(&mdkey, &k).unwrap(); + assert_eq!(m.get_metadata_raw(&mdkey, &k).unwrap(), None); + + m.unset_key(&k, Committed::Live).unwrap(); + assert_eq!(m.get_key(&k, Committed::Live).unwrap(), None); } #[test] diff --git a/workspaces/api/apiserver/src/datastore/mod.rs b/workspaces/api/apiserver/src/datastore/mod.rs index baf5c3776d4..a5bac35bfbc 100644 --- a/workspaces/api/apiserver/src/datastore/mod.rs +++ b/workspaces/api/apiserver/src/datastore/mod.rs @@ -13,8 +13,7 @@ pub mod deserialization; pub mod error; pub mod filesystem; pub mod key; -#[cfg(test)] -pub(crate) mod memory; +pub mod memory; pub mod serialization; pub use error::{Error, Result}; @@ -61,6 +60,10 @@ pub trait DataStore { fn get_key(&self, key: &Key, committed: Committed) -> Result>; /// Set the value of a single data key in the datastore. fn set_key>(&mut self, key: &Key, value: S, committed: Committed) -> Result<()>; + /// Removes the given data key from the datastore. If we succeeded, we return Ok(()); if + /// the key didn't exist, we also return Ok(()); we return Err only if we failed to check + /// or remove the key. + fn unset_key(&mut self, key: &Key, committed: Committed) -> Result<()>; /// Retrieve the value for a single metadata key from the datastore. Values will inherit from /// earlier in the tree, if more specific values are not found later. @@ -93,6 +96,10 @@ pub trait DataStore { data_key: &Key, value: S, ) -> Result<()>; + /// Removes the given metadata key from the given data key in the datastore. If we + /// succeeded, we return Ok(()); if the data or metadata key didn't exist, we also return + /// Ok(()); we return Err only if we failed to check or remove the key. + fn unset_metadata(&mut self, metadata_key: &Key, data_key: &Key) -> Result<()>; /// Applies pending changes to the live datastore. Returns the list of changed keys. fn commit(&mut self) -> Result>; @@ -114,6 +121,17 @@ pub trait DataStore { } Ok(()) } + /// Removes multiple data keys at once in the data store. + /// + /// Implementers can replace the default implementation if there's a faster way than + /// unsetting each key individually. + fn unset_keys(&mut self, keys: &HashSet, committed: Committed) -> Result<()> { + for key in keys { + trace!("Unsetting data key {}", key.name()); + self.unset_key(key, committed)?; + } + Ok(()) + } /// Retrieves all keys starting with the given prefix, returning them in a Key -> value map. /// @@ -237,25 +255,36 @@ pub type Value = serde_json::Value; mod test { use super::memory::MemoryDataStore; use super::{Committed, DataStore, Key, KeyType}; - use maplit::hashmap; + use maplit::{hashmap, hashset}; #[test] - fn set_keys() { + fn set_unset_keys() { let mut m = MemoryDataStore::new(); let k1 = Key::new(KeyType::Data, "memtest1").unwrap(); let k2 = Key::new(KeyType::Data, "memtest2").unwrap(); + let k3 = Key::new(KeyType::Data, "memtest3").unwrap(); let v1 = "memvalue1".to_string(); let v2 = "memvalue2".to_string(); + let v3 = "memvalue3".to_string(); let data = hashmap!( k1.clone() => &v1, k2.clone() => &v2, + k3.clone() => &v3, ); m.set_keys(&data, Committed::Pending).unwrap(); assert_eq!(m.get_key(&k1, Committed::Pending).unwrap(), Some(v1)); assert_eq!(m.get_key(&k2, Committed::Pending).unwrap(), Some(v2)); + assert_eq!(m.get_key(&k3, Committed::Pending).unwrap(), Some(v3.clone())); + + let unset = hashset!(k1.clone(), k2.clone()); + m.unset_keys(&unset, Committed::Pending).unwrap(); + + assert_eq!(m.get_key(&k1, Committed::Pending).unwrap(), None); + assert_eq!(m.get_key(&k2, Committed::Pending).unwrap(), None); + assert_eq!(m.get_key(&k3, Committed::Pending).unwrap(), Some(v3)); } #[test] diff --git a/workspaces/api/migration/migration-helpers/Cargo.toml b/workspaces/api/migration/migration-helpers/Cargo.toml index ba880820c25..c5d4e32b408 100644 --- a/workspaces/api/migration/migration-helpers/Cargo.toml +++ b/workspaces/api/migration/migration-helpers/Cargo.toml @@ -9,3 +9,7 @@ publish = false apiserver = { path = "../../apiserver" } snafu = "0.6" toml = "0.5" + +[dev-dependencies] +maplit = "1.0" +serde_json = "1.0" diff --git a/workspaces/api/migration/migration-helpers/src/args.rs b/workspaces/api/migration/migration-helpers/src/args.rs index 09263becb86..3b96b754c0c 100644 --- a/workspaces/api/migration/migration-helpers/src/args.rs +++ b/workspaces/api/migration/migration-helpers/src/args.rs @@ -7,7 +7,8 @@ use crate::{MigrationType, Result}; /// Stores user-supplied arguments. pub struct Args { - pub datastore_path: String, + pub source_datastore: String, + pub target_datastore: String, pub migration_type: MigrationType, } @@ -16,7 +17,8 @@ fn usage() -> ! { let program_name = env::args().next().unwrap_or_else(|| "program".to_string()); eprintln!( r"Usage: {} - --datastore-path PATH + --source-datastore PATH + --target-datastore PATH ( --forward | --backward )", program_name ); @@ -32,16 +34,33 @@ fn usage_msg>(msg: S) -> ! { /// Parses user arguments into an Args structure. pub(crate) fn parse_args(args: env::Args) -> Result { let mut migration_type = None; + let mut source_datastore = None; + let mut target_datastore = None; let mut datastore_path = None; let mut iter = args.skip(1); while let Some(arg) = iter.next() { match arg.as_ref() { + "--source-datastore" => { + source_datastore = + Some(iter.next().unwrap_or_else(|| { + usage_msg("Did not give argument to --source-datastore") + })) + } + + "--target-datastore" => { + target_datastore = + Some(iter.next().unwrap_or_else(|| { + usage_msg("Did not give argument to --target-datastore") + })) + } + + // Support the argument of the old migrator interface, with some caveats "--datastore-path" => { datastore_path = Some( iter.next() .unwrap_or_else(|| usage_msg("Did not give argument to --datastore-path")), - ) + ); } "--forward" => migration_type = Some(MigrationType::Forward), @@ -51,8 +70,25 @@ pub(crate) fn parse_args(args: env::Args) -> Result { } } + if let Some(datastore_path) = datastore_path { + // For compatibility with old migration interface that had single source+target; other code + // in migration-helpers checks if source==target to see if it needs to do a workaround. + if source_datastore.is_some() || target_datastore.is_some() { + usage_msg("--datastore-path is only for backward compatibility and \ + cannot be used with --source-datastore / --target-datastore"); + } + source_datastore = Some(datastore_path.clone()); + target_datastore = Some(datastore_path); + } else { + // In no other case should they be the same; we use it for compatibility checks. + if source_datastore == target_datastore { + usage_msg("--source-datastore and --target-datastore cannot be the same"); + } + } + Ok(Args { - datastore_path: datastore_path.unwrap_or_else(|| usage()), + source_datastore: source_datastore.unwrap_or_else(|| usage()), + target_datastore: target_datastore.unwrap_or_else(|| usage()), migration_type: migration_type.unwrap_or_else(|| usage()), }) } diff --git a/workspaces/api/migration/migration-helpers/src/error.rs b/workspaces/api/migration/migration-helpers/src/error.rs index e0228447d6a..5f743441136 100644 --- a/workspaces/api/migration/migration-helpers/src/error.rs +++ b/workspaces/api/migration/migration-helpers/src/error.rs @@ -29,6 +29,12 @@ pub enum Error { #[snafu(display("Unable to write to data store: {}", source))] DataStoreWrite { source: datastore::Error }, + #[snafu(display("Unable to remove key '{}' from data store: {}", key, source))] + DataStoreRemove { + key: String, + source: datastore::Error, + }, + #[snafu(display("Migrated data failed validation: {}", msg))] Validation { msg: String }, diff --git a/workspaces/api/migration/migration-helpers/src/lib.rs b/workspaces/api/migration/migration-helpers/src/lib.rs index dbdb00ecaa0..7994eab3281 100644 --- a/workspaces/api/migration/migration-helpers/src/lib.rs +++ b/workspaces/api/migration/migration-helpers/src/lib.rs @@ -14,6 +14,7 @@ mod args; mod datastore; pub mod error; +mod workarounds; use std::collections::HashMap; use std::env; @@ -22,9 +23,10 @@ use std::fmt; use apiserver::datastore::{Committed, Value}; pub use apiserver::datastore::{DataStore, FilesystemDataStore}; -use args::parse_args; +use args::{parse_args, Args}; use datastore::{get_input_data, set_output_data}; pub use error::Result; +use workarounds::fix_migrated_data; /// The data store implementation currently in use. Used by the simpler `migrate` interface; can /// be overridden by using the `run_migration` interface. @@ -57,7 +59,7 @@ pub type Metadata = HashMap; /// MigrationData holds all data that can be migrated in a migration, and serves as the input and /// output format of migrations. A serde Value type is used to hold the arbitrary data of each /// key because we can't represent types when they could change in the migration. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct MigrationData { /// Mapping of data key names to their arbitrary values. pub data: HashMap, @@ -83,22 +85,31 @@ fn validate_migrated_data(_migrated: &MigrationData) -> Result<()> { /// If you need a little more control over a migration than with migrate, or you're using this /// module as a library, you can call run_migration directly with the arguments that would /// normally be parsed from the migration binary's command line. -pub fn run_migration( - mut migration: impl Migration, - datastore: &mut D, - migration_type: MigrationType, -) -> Result<()> { +pub fn run_migration(mut migration: impl Migration, args: &Args) -> Result<()> { + let source = DataStoreImplementation::new(&args.source_datastore); + let mut target = DataStoreImplementation::new(&args.target_datastore); + for committed in &[Committed::Live, Committed::Pending] { - let input = get_input_data(datastore, *committed)?; + let input = get_input_data(&source, *committed)?; - let migrated = match migration_type { - MigrationType::Forward => migration.forward(input), - MigrationType::Backward => migration.backward(input), + let mut migrated = input.clone(); + migrated = match args.migration_type { + MigrationType::Forward => migration.forward(migrated), + MigrationType::Backward => migration.backward(migrated), }?; + fix_migrated_data( + &input, + &mut migrated, + &source, + &mut target, + *committed, + &args, + )?; + validate_migrated_data(&migrated)?; - set_output_data(datastore, &migrated, *committed)?; + set_output_data(&mut target, &migrated, *committed)?; } Ok(()) } @@ -121,10 +132,9 @@ impl fmt::Display for MigrationType { /// This is the primary entry point for migration authors. When you've implemented the Migration /// trait, you should just be able to pass it to this function from your main function and let it -/// take care of the rest. The migration runner will pass in the appropriate datastore path and +/// take care of the rest. The migration runner will pass in the appropriate datastore paths and /// migration type. pub fn migrate(migration: impl Migration) -> Result<()> { let args = parse_args(env::args())?; - let mut datastore = DataStoreImplementation::new(args.datastore_path); - run_migration(migration, &mut datastore, args.migration_type) + run_migration(migration, &args) } diff --git a/workspaces/api/migration/migration-helpers/src/workarounds.rs b/workspaces/api/migration/migration-helpers/src/workarounds.rs new file mode 100644 index 00000000000..420668ae9ce --- /dev/null +++ b/workspaces/api/migration/migration-helpers/src/workarounds.rs @@ -0,0 +1,168 @@ +use crate::args::Args; +use crate::{error, MigrationData, Result}; +use apiserver::datastore::{Committed, DataStore, Key, KeyType}; +use snafu::ResultExt; +use std::collections::HashSet; + +/// Here we can fix known issues with migrated data, for example issues related to changes +/// in migration interface that we don't want the migrations to have to deal with. +pub(crate) fn fix_migrated_data( + input: &MigrationData, + output: &MigrationData, + _source_datastore: &D, + target_datastore: &mut D, + committed: Committed, + args: &Args, +) -> Result<()> { + // If the source and target data store path are the same, we're using the old migrator + // interface, and have to use a workaround to be able to delete keys. They can't just be + // removed from the MigrationData struct, because the old interface used the same data store + // for input and output, and removing from MigrationData just means we won't write it out + // again - but the file will still be there from the input. We need to tell the data store + // to remove it. + if args.source_datastore == args.target_datastore { + // Data keys first + let old_keys: HashSet<_> = input.data.keys().collect(); + let new_keys: HashSet<_> = output.data.keys().collect(); + for removed_key_str in old_keys.difference(&new_keys) { + // We need to make a Key from the key's name to fit the data store interface; we + // don't use Key in MigrationData for the convenience of migration authors. + let removed_key = + Key::new(KeyType::Data, removed_key_str).context(error::InvalidKey { + key_type: KeyType::Data, + key: *removed_key_str, + })?; + target_datastore + .unset_key(&removed_key, committed) + .context(error::DataStoreRemove { + key: *removed_key_str, + })?; + } + + // Now the same thing for metadata + for (data_key_str, old_metadata) in &input.metadata { + let removed: HashSet<_> = if let Some(new_metadata) = output.metadata.get(data_key_str) + { + // Find which metadata keys the migration removed, for this data key + let old_keys: HashSet<_> = old_metadata.keys().collect(); + let new_keys: HashSet<_> = new_metadata.keys().collect(); + old_keys.difference(&new_keys).map(|&s| s).collect() + } else { + // Migration output has no metadata for this data key, so it was all removed + old_metadata.keys().collect() + }; + + for removed_meta_str in removed { + let removed_meta = + Key::new(KeyType::Meta, removed_meta_str).context(error::InvalidKey { + key_type: KeyType::Meta, + key: removed_meta_str, + })?; + let removed_data = + Key::new(KeyType::Data, data_key_str).context(error::InvalidKey { + key_type: KeyType::Data, + key: data_key_str, + })?; + target_datastore + .unset_metadata(&removed_meta, &removed_data) + .context(error::DataStoreRemove { + key: removed_meta_str, + })?; + } + } + } + + Ok(()) +} + +#[cfg(test)] +mod test { + use super::fix_migrated_data; + use crate::datastore::set_output_data; + use crate::{Args, MigrationData, MigrationType}; + use apiserver::datastore::memory::MemoryDataStore; + use apiserver::datastore::{Committed, DataStore, Key, KeyType}; + use maplit::hashmap; + use serde_json::json; + + #[test] + fn test_fix_migrated_data() { + // Data/metadata starting with "remove" should be removed + let input = MigrationData { + data: hashmap!( + "keepdata".into() => json!("hi"), + "removedata".into() => json!("sup"), + ), + metadata: hashmap!( + "keepdata".into() => hashmap!( + "keepmeta".into() => json!("howdy"), + "removemeta".into() => json!("yo"), + ), + "removedata".into() => hashmap!( + "keepmeta".into() => json!("hello"), + "removemeta".into() => json!("hiya"), + ), + ), + }; + // This represents 'input' after a migration removes some data, so it should match the + // data store after we call fix_migrated_data + let expected = MigrationData { + data: hashmap!( + "keepdata".into() => json!("hi"), + ), + metadata: hashmap!( + "keepdata".into() => hashmap!( + "keepmeta".into() => json!("howdy"), + ), + "removedata".into() => hashmap!( + "keepmeta".into() => json!("hello"), + ), + ), + }; + + // The point of the workaround is affecting the data store directly, so make test stores + let mut source = MemoryDataStore::new(); + set_output_data(&mut source, &input, Committed::Live).unwrap(); + // To replicate old interface, the target data store starts with the input data, and + // we're going to confirm that removed values are actually removed + let mut target = MemoryDataStore::new(); + set_output_data(&mut target, &input, Committed::Live).unwrap(); + + // Ensure values are there at the start + let kept_data = Key::new(KeyType::Data, "keepdata").unwrap(); + let removed_data = Key::new(KeyType::Data, "removedata").unwrap(); + let kept_meta = Key::new(KeyType::Meta, "keepmeta").unwrap(); + let removed_meta = Key::new(KeyType::Meta, "removemeta").unwrap(); + assert_eq!(target.get_key(&kept_data, Committed::Live).unwrap(), Some("\"hi\"".into())); + assert_eq!(target.get_key(&removed_data, Committed::Live).unwrap(), Some("\"sup\"".into())); + assert_eq!(target.get_metadata(&kept_meta, &kept_data).unwrap(), Some("\"howdy\"".into())); + assert_eq!(target.get_metadata(&kept_meta, &removed_data).unwrap(), Some("\"hello\"".into())); + assert_eq!(target.get_metadata(&removed_meta, &kept_data).unwrap(), Some("\"yo\"".into())); + assert_eq!(target.get_metadata(&removed_meta, &removed_data).unwrap(), Some("\"hiya\"".into())); + + // Same source and target, i.e. using old interface, so we should do our fix + let args = Args { + source_datastore: "same".into(), + target_datastore: "same".into(), + migration_type: MigrationType::Forward, + }; + fix_migrated_data( + &input, + &expected, + &source, + &mut target, + Committed::Live, + &args, + ) + .unwrap(); + + // Ensure unaffected values were kept + assert_eq!(target.get_key(&kept_data, Committed::Live).unwrap(), Some("\"hi\"".into())); + assert_eq!(target.get_metadata(&kept_meta, &kept_data).unwrap(), Some("\"howdy\"".into())); + assert_eq!(target.get_metadata(&kept_meta, &removed_data).unwrap(), Some("\"hello\"".into())); + // Ensure removed values were removed + assert_eq!(target.get_key(&removed_data, Committed::Live).unwrap(), None); + assert_eq!(target.get_metadata(&removed_meta, &kept_data).unwrap(), None); + assert_eq!(target.get_metadata(&removed_meta, &removed_data).unwrap(), None); + } +} diff --git a/workspaces/api/migration/migrator/Cargo.toml b/workspaces/api/migration/migrator/Cargo.toml index a9ac502577d..c671a6b024e 100644 --- a/workspaces/api/migration/migrator/Cargo.toml +++ b/workspaces/api/migration/migrator/Cargo.toml @@ -8,7 +8,6 @@ build = "build.rs" [dependencies] data_store_version = { path = "../../data_store_version" } -fs_extra = "1.1" lazy_static = "1.2" log = "0.4" nix = "0.16" diff --git a/workspaces/api/migration/migrator/src/error.rs b/workspaces/api/migration/migrator/src/error.rs index 4feefa36dbb..9563f7b8236 100644 --- a/workspaces/api/migration/migrator/src/error.rs +++ b/workspaces/api/migration/migrator/src/error.rs @@ -46,9 +46,6 @@ pub(crate) enum Error { #[snafu(display("Data store for new version {} already exists at {}", version, path.display()))] NewVersionAlreadyExists { version: Version, path: PathBuf }, - #[snafu(display("Failed copying data store to work location: {}", source))] - DataStoreCopy { source: fs_extra::error::Error }, - #[snafu(display("Unable to start migration command {:?} - {}", command, source))] StartMigration { command: Command, source: io::Error }, diff --git a/workspaces/api/migration/migrator/src/main.rs b/workspaces/api/migration/migrator/src/main.rs index af630e034c8..bfb1b2e1a09 100644 --- a/workspaces/api/migration/migrator/src/main.rs +++ b/workspaces/api/migration/migrator/src/main.rs @@ -93,7 +93,7 @@ fn run() -> Result<()> { )?; let (copy_path, copy_id) = copy_datastore(&args.datastore_path, args.migrate_to_version)?; - run_migrations(direction, &migrations, ©_path)?; + run_migrations(direction, &migrations, &args.datastore_path, ©_path)?; flip_to_new_minor_version(args.migrate_to_version, ©_path, ©_id)?; Ok(()) @@ -276,28 +276,17 @@ fn copy_datastore>(from: P, new_version: Version) -> Result<(Path } ); - info!( - "Copying datastore from {} to work location {}", - from.as_ref().display(), - to.display() - ); - - let mut copy_options = fs_extra::dir::CopyOptions::new(); - // Set copy_inside true; if we're moving from v0.0 to v0.1, and this isn't set, it tries to - // copy "v0.0" itself inside "v0.1". - copy_options.copy_inside = true; - // Note: this copies file permissions but not directory permissions; OK? - fs_extra::dir::copy(&from, &to, ©_options).context(error::DataStoreCopy)?; - + info!("New data store is being built at work location {}", to.display()); Ok((to, copy_id)) } /// Runs the given migrations in their given order on the given data store. The given direction /// is passed to each migration so it knows which direction we're migrating. -fn run_migrations(direction: Direction, migrations: &[P1], datastore_path: P2) -> Result<()> +fn run_migrations(direction: Direction, migrations: &[P1], source_datastore: P2, target_datastore: P3) -> Result<()> where P1: AsRef, P2: AsRef, + P3: AsRef, { for migration in migrations { // Ensure the migration is executable. @@ -312,8 +301,12 @@ where // Point each migration in the right direction, and at the given data store. command.arg(direction.to_string()); command.args(&[ - "--datastore-path".to_string(), - datastore_path.as_ref().display().to_string(), + "--source-datastore".to_string(), + source_datastore.as_ref().display().to_string(), + ]); + command.args(&[ + "--target-datastore".to_string(), + target_datastore.as_ref().display().to_string(), ]); info!("Running migration command: {:?}", command);