diff --git a/.circleci/config.yml b/.circleci/config.yml index f961a8566d..188db596f3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -63,11 +63,6 @@ commands: command: | export GLEAN_TEST_COVERAGE=$(realpath glean_coverage.txt) cargo test --verbose --jobs 6 -- --nocapture - - run: - name: Test Glean with rkv safe-mode - command: | - cd glean-core - cargo test -p glean-core --features rkv-safe-mode --jobs 6 -- --nocapture - run: name: Install required Python dependencies command: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e1d1dabbf..6688d0b8d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ (`null`, `nil`, `None` depending on the language) and does not throw an exception ([#2087](https://github.com/mozilla/glean/pull/2087)). * BREAKING CHANGE: Dropped `ping_name` argument from all `test_get_num_recorded_errors` methods ([#2088](https://github.com/mozilla/glean/pull/2088)) Errors default to the `metrics` ping, so that's what is queried internally. + * BREAKING: Disable `safe-mode` everywhere. This causes all clients to migrate from LMDB to safe-mode storage ([#2123](https://github.com/mozilla/glean/pull/2123)) * Kotlin * Fix the Glean Gradle Plugin to work with Android Gradle Plugin v7.2.1 ([#2114](https://github.com/mozilla/glean/pull/2114)) * Rust diff --git a/glean-core/Cargo.toml b/glean-core/Cargo.toml index 920445cedf..097b63f4c2 100644 --- a/glean-core/Cargo.toml +++ b/glean-core/Cargo.toml @@ -61,7 +61,3 @@ ctor = "0.1.12" [build-dependencies] uniffi_build = { version = "0.19.3", features = ["builtin-bindgen"] } - -[features] -# Enable the "safe-mode" Rust storage backend instead of the default LMDB one. -rkv-safe-mode = [] diff --git a/glean-core/bundle-android/Cargo.toml b/glean-core/bundle-android/Cargo.toml index 6a493692e6..c1ee55e6aa 100644 --- a/glean-core/bundle-android/Cargo.toml +++ b/glean-core/bundle-android/Cargo.toml @@ -23,7 +23,3 @@ path = "../bundle/src/lib.rs" [dependencies.glean-core] # No version specified, we build against what's available here. path = ".." - -[features] -# Enable the "safe-mode" Rust storage backend instead of the default LMDB one. -rkv-safe-mode = ["glean-core/rkv-safe-mode"] diff --git a/glean-core/bundle/Cargo.toml b/glean-core/bundle/Cargo.toml index 4005c3f374..9b48ce142d 100644 --- a/glean-core/bundle/Cargo.toml +++ b/glean-core/bundle/Cargo.toml @@ -20,7 +20,3 @@ crate-type = ["staticlib", "cdylib"] [dependencies.glean-core] # No version specified, we build against what's available here. path = ".." - -[features] -# Enable the "safe-mode" Rust storage backend instead of the default LMDB one. -rkv-safe-mode = ["glean-core/rkv-safe-mode"] diff --git a/glean-core/python/setup.py b/glean-core/python/setup.py index 775535ff2d..81a056abe3 100644 --- a/glean-core/python/setup.py +++ b/glean-core/python/setup.py @@ -182,8 +182,6 @@ def run(self): "glean-bundle", "--target", target, - "--features", - "rkv-safe-mode", ] if buildvariant != "debug": command.append(f"--{buildvariant}") diff --git a/glean-core/rlb/Cargo.toml b/glean-core/rlb/Cargo.toml index abece09ee8..fc2d9c7af4 100644 --- a/glean-core/rlb/Cargo.toml +++ b/glean-core/rlb/Cargo.toml @@ -42,7 +42,3 @@ env_logger = { version = "0.9.0", default-features = false, features = ["termcol tempfile = "3.1.0" jsonschema-valid = "0.5.0" flate2 = "1.0.19" - -[features] -# Enable the "safe-mode" Rust storage backend instead of the default LMDB one. -rkv-safe-mode = ["glean-core/rkv-safe-mode"] diff --git a/glean-core/src/database/mod.rs b/glean-core/src/database/mod.rs index bdad7ee0c2..2d10ed7191 100644 --- a/glean-core/src/database/mod.rs +++ b/glean-core/src/database/mod.rs @@ -10,6 +10,7 @@ use std::path::Path; use std::str; use std::sync::RwLock; +use rkv::migrator::Migrator; use rkv::StoreOptions; /// Unwrap a `Result`s `Ok` value or do the specified action. @@ -27,148 +28,119 @@ macro_rules! unwrap_or { }; } -// Select the LMDB-powered storage backend when the feature is not activated. -#[cfg(not(feature = "rkv-safe-mode"))] -mod backend { - use std::path::Path; - - /// cbindgen:ignore - pub type Rkv = rkv::Rkv; - /// cbindgen:ignore - pub type SingleStore = rkv::SingleStore; - /// cbindgen:ignore - pub type Writer<'t> = rkv::Writer>; - - pub fn rkv_new(path: &Path) -> Result { - Rkv::new::(path) - } - - /// No migration necessary when staying with LMDB. - pub fn migrate(_path: &Path, _dst_env: &Rkv) { - // Intentionally left empty. +/// cbindgen:ignore +pub type Rkv = rkv::Rkv; +/// cbindgen:ignore +pub type SingleStore = rkv::SingleStore; +/// cbindgen:ignore +pub type Writer<'t> = rkv::Writer>; + +pub fn rkv_new(path: &Path) -> std::result::Result { + match Rkv::new::(path) { + // An invalid file can mean: + // 1. An empty file. + // 2. A corrupted file. + // + // In both instances there's not much we can do. + // Drop the data by removing the file, and start over. + Err(rkv::StoreError::FileInvalid) => { + let safebin = path.join("data.safe.bin"); + fs::remove_file(safebin).map_err(|_| rkv::StoreError::FileInvalid)?; + // Now try again, we only handle that error once. + Rkv::new::(path) + } + other => other, } } -// Select the "safe mode" storage backend when the feature is activated. -#[cfg(feature = "rkv-safe-mode")] -mod backend { - use rkv::migrator::Migrator; - use std::{fs, path::Path}; - - /// cbindgen:ignore - pub type Rkv = rkv::Rkv; - /// cbindgen:ignore - pub type SingleStore = rkv::SingleStore; - /// cbindgen:ignore - pub type Writer<'t> = rkv::Writer>; - - pub fn rkv_new(path: &Path) -> Result { - match Rkv::new::(path) { - // An invalid file can mean: - // 1. An empty file. - // 2. A corrupted file. - // - // In both instances there's not much we can do. - // Drop the data by removing the file, and start over. - Err(rkv::StoreError::FileInvalid) => { - let safebin = path.join("data.safe.bin"); - fs::remove_file(safebin).map_err(|_| rkv::StoreError::FileInvalid)?; - // Now try again, we only handle that error once. - Rkv::new::(path) +fn delete_and_log(path: &Path, msg: &str) { + if let Err(err) = fs::remove_file(path) { + match err.kind() { + std::io::ErrorKind::NotFound => { + // Silently drop this error, the file was already non-existing. } - other => other, + _ => log::warn!("{}", msg), } } +} - fn delete_and_log(path: &Path, msg: &str) { - if let Err(err) = fs::remove_file(path) { - match err.kind() { - std::io::ErrorKind::NotFound => { - // Silently drop this error, the file was already non-existing. - } - _ => log::warn!("{}", msg), - } - } - } +fn delete_lmdb_database(path: &Path) { + let datamdb = path.join("data.mdb"); + delete_and_log(&datamdb, "Failed to delete old data."); - fn delete_lmdb_database(path: &Path) { - let datamdb = path.join("data.mdb"); - delete_and_log(&datamdb, "Failed to delete old data."); + let lockmdb = path.join("lock.mdb"); + delete_and_log(&lockmdb, "Failed to delete old lock."); +} - let lockmdb = path.join("lock.mdb"); - delete_and_log(&lockmdb, "Failed to delete old lock."); +/// Migrate from LMDB storage to safe-mode storage. +/// +/// This migrates the data once, then deletes the LMDB storage. +/// The safe-mode storage must be empty for it to work. +/// Existing data will not be overwritten. +/// If the destination database is not empty the LMDB database is deleted +/// without migrating data. +/// This is a no-op if no LMDB database file exists. +pub fn migrate(path: &Path, dst_env: &Rkv) { + use rkv::{MigrateError, StoreError}; + + log::debug!("Migrating files in {}", path.display()); + + // Shortcut if no data to migrate is around. + let datamdb = path.join("data.mdb"); + if !datamdb.exists() { + log::debug!("No data to migrate."); + return; } - /// Migrate from LMDB storage to safe-mode storage. - /// - /// This migrates the data once, then deletes the LMDB storage. - /// The safe-mode storage must be empty for it to work. - /// Existing data will not be overwritten. - /// If the destination database is not empty the LMDB database is deleted - /// without migrating data. - /// This is a no-op if no LMDB database file exists. - pub fn migrate(path: &Path, dst_env: &Rkv) { - use rkv::{MigrateError, StoreError}; - - log::debug!("Migrating files in {}", path.display()); - - // Shortcut if no data to migrate is around. - let datamdb = path.join("data.mdb"); - if !datamdb.exists() { - log::debug!("No data to migrate."); - return; - } - - // We're handling the same error cases as `easy_migrate_lmdb_to_safe_mode`, - // but annotate each why they don't cause problems for Glean. - // Additionally for known cases we delete the LMDB database regardless. - let should_delete = - match Migrator::open_and_migrate_lmdb_to_safe_mode(path, |builder| builder, dst_env) { - // Source environment is corrupted. - // We start fresh with the new database. - Err(MigrateError::StoreError(StoreError::FileInvalid)) => true, - Err(MigrateError::StoreError(StoreError::DatabaseCorrupted)) => true, - // Path not accessible. - // Somehow our directory vanished between us creating it and reading from it. - // Nothing we can do really. - Err(MigrateError::StoreError(StoreError::IoError(_))) => true, - // Path accessible but incompatible for configuration. - // This should not happen, we never used storages that safe-mode doesn't understand. - // If it does happen, let's start fresh and use the safe-mode from now on. - Err(MigrateError::StoreError(StoreError::UnsuitableEnvironmentPath(_))) => true, - // Nothing to migrate. - // Source database was empty. We just start fresh anyway. - Err(MigrateError::SourceEmpty) => true, - // Migrating would overwrite. - // Either a previous migration failed and we still started writing data, - // or someone placed back an old data file. - // In any case we better stay on the new data and delete the old one. - Err(MigrateError::DestinationNotEmpty) => { - log::warn!("Failed to migrate old data. Destination was not empty"); - true - } - // An internal lock was poisoned. - // This would only happen if multiple things run concurrently and one crashes. - Err(MigrateError::ManagerPoisonError) => false, - // Couldn't close source environment and delete files on disk (e.g. other stores still open). - // This could only happen if multiple instances are running, - // we leave files in place. - Err(MigrateError::CloseError(_)) => false, - // Other store errors are never returned from the migrator. - // We need to handle them to please rustc. - Err(MigrateError::StoreError(_)) => false, - // Other errors can't happen, so this leaves us with the Ok case. - // This already deleted the LMDB files. - Ok(()) => false, - }; - - if should_delete { - log::debug!("Need to delete remaining LMDB files."); - delete_lmdb_database(path); - } + // We're handling the same error cases as `easy_migrate_lmdb_to_safe_mode`, + // but annotate each why they don't cause problems for Glean. + // Additionally for known cases we delete the LMDB database regardless. + let should_delete = + match Migrator::open_and_migrate_lmdb_to_safe_mode(path, |builder| builder, dst_env) { + // Source environment is corrupted. + // We start fresh with the new database. + Err(MigrateError::StoreError(StoreError::FileInvalid)) => true, + Err(MigrateError::StoreError(StoreError::DatabaseCorrupted)) => true, + // Path not accessible. + // Somehow our directory vanished between us creating it and reading from it. + // Nothing we can do really. + Err(MigrateError::StoreError(StoreError::IoError(_))) => true, + // Path accessible but incompatible for configuration. + // This should not happen, we never used storages that safe-mode doesn't understand. + // If it does happen, let's start fresh and use the safe-mode from now on. + Err(MigrateError::StoreError(StoreError::UnsuitableEnvironmentPath(_))) => true, + // Nothing to migrate. + // Source database was empty. We just start fresh anyway. + Err(MigrateError::SourceEmpty) => true, + // Migrating would overwrite. + // Either a previous migration failed and we still started writing data, + // or someone placed back an old data file. + // In any case we better stay on the new data and delete the old one. + Err(MigrateError::DestinationNotEmpty) => { + log::warn!("Failed to migrate old data. Destination was not empty"); + true + } + // An internal lock was poisoned. + // This would only happen if multiple things run concurrently and one crashes. + Err(MigrateError::ManagerPoisonError) => false, + // Couldn't close source environment and delete files on disk (e.g. other stores still open). + // This could only happen if multiple instances are running, + // we leave files in place. + Err(MigrateError::CloseError(_)) => false, + // Other store errors are never returned from the migrator. + // We need to handle them to please rustc. + Err(MigrateError::StoreError(_)) => false, + // Other errors can't happen, so this leaves us with the Ok case. + // This already deleted the LMDB files. + Ok(()) => false, + }; - log::debug!("Migration ended. Safe-mode database in {}", path.display()); + if should_delete { + log::debug!("Need to delete remaining LMDB files."); + delete_lmdb_database(path); } + + log::debug!("Migration ended. Safe-mode database in {}", path.display()); } use crate::metrics::Metric; @@ -176,7 +148,6 @@ use crate::CommonMetricData; use crate::Glean; use crate::Lifetime; use crate::Result; -use backend::*; pub struct Database { /// Handle to the database environment. @@ -250,21 +221,6 @@ impl Database { /// It also loads any Lifetime::Ping data that might be /// persisted, in case `delay_ping_lifetime_io` is set. pub fn new(data_path: &Path, delay_ping_lifetime_io: bool) -> Result { - #[cfg(all(windows, not(feature = "rkv-safe-mode")))] - { - // The underlying lmdb wrapper implementation - // cannot actually handle non-UTF8 paths on Windows. - // It will unconditionally panic if passed one. - // See - // https://github.com/mozilla/lmdb-rs/blob/df1c2f56e3088f097c719c57b9925ab51e26f3f4/src/environment.rs#L43-L53 - // - // To avoid this, in case we're using LMDB on Windows (that's just testing now though), - // we simply error out earlier. - if data_path.to_str().is_none() { - return Err(crate::Error::utf8_error()); - } - } - let path = data_path.join("db"); log::debug!("Database path: {:?}", path.display()); let file_size = database_size(&path); @@ -839,25 +795,12 @@ mod test { let res = Database::new(&path, false); - #[cfg(feature = "rkv-safe-mode")] - { - assert!( - res.is_ok(), - "Database should succeed at {}: {:?}", - path.display(), - res - ); - } - - #[cfg(not(feature = "rkv-safe-mode"))] - { - assert!( - res.is_err(), - "Database should fail at {}: {:?}", - path.display(), - res - ); - } + assert!( + res.is_ok(), + "Database should succeed at {}: {:?}", + path.display(), + res + ); } #[test] @@ -1491,27 +1434,6 @@ mod test { ); } - /// LDMB ignores an empty database file just fine. - #[cfg(not(feature = "rkv-safe-mode"))] - #[test] - fn empty_data_file() { - let dir = tempdir().unwrap(); - - // Create database directory structure. - let database_dir = dir.path().join("db"); - fs::create_dir_all(&database_dir).expect("create database dir"); - - // Create empty database file. - let datamdb = database_dir.join("data.mdb"); - let f = fs::File::create(datamdb).expect("create database file"); - drop(f); - - Database::new(dir.path(), false).unwrap(); - - assert!(dir.path().exists()); - } - - #[cfg(feature = "rkv-safe-mode")] mod safe_mode { use std::fs::File; diff --git a/samples/rust/Cargo.toml b/samples/rust/Cargo.toml index 34ee186a05..35f62d8d68 100644 --- a/samples/rust/Cargo.toml +++ b/samples/rust/Cargo.toml @@ -7,7 +7,7 @@ license = "MIT" [dependencies] env_logger = { version = "0.9.0", default-features = false, features = ["termcolor", "atty", "humantime"] } -glean = { path = "../../glean-core/rlb", features = ["rkv-safe-mode"] } +glean = { path = "../../glean-core/rlb" } tempfile = "3.3.0" [build-dependencies]