-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add opt in automatic database migration #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,8 +14,9 @@ mod types; | |||||
|
|
||||||
| use crate::app::RuntimeApp; | ||||||
| use crate::config::Config; | ||||||
| use anyhow::Result; | ||||||
| use anyhow::{Context, Result}; | ||||||
| use app::App; | ||||||
| use config::DbUpgrade; | ||||||
| use db::node_nic::ReplaceNic; | ||||||
| use license::LicenseVerifier; | ||||||
| use shared::bee_msg::target::RefreshTargetStates; | ||||||
|
|
@@ -82,8 +83,19 @@ pub async fn start(info: StaticInfo, license: LicenseVerifier) -> Result<RunCont | |||||
| info.use_ipv6, | ||||||
| ); | ||||||
|
|
||||||
| let mut db = sqlite::Connections::new(info.user_config.db_file.as_path()); | ||||||
| sqlite::check_schema_async(&mut db, db::MIGRATIONS).await?; | ||||||
| let db = sqlite::Connections::new(info.user_config.db_file.as_path()); | ||||||
|
|
||||||
| let schema_check = db | ||||||
| .read_tx(|tx| Ok(sqlite::check_schema(tx, db::MIGRATIONS))) | ||||||
| .await?; | ||||||
|
|
||||||
| if info.user_config.db_upgrade == DbUpgrade::Auto { | ||||||
| if schema_check.is_err() { | ||||||
| upgrade_db(&db).await?; | ||||||
| } | ||||||
|
Comment on lines
+93
to
+95
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion(non-blocking): This would try to upgrade the DB on any error from the The migration would never actually be attempted due to the second check in |
||||||
| } else { | ||||||
| schema_check?; | ||||||
| } | ||||||
|
|
||||||
| log::info!( | ||||||
| "Opened database at {:?}", | ||||||
|
|
@@ -152,6 +164,27 @@ pub async fn start(info: StaticInfo, license: LicenseVerifier) -> Result<RunCont | |||||
| }) | ||||||
| } | ||||||
|
|
||||||
| // Db schema auto upgrade. This requires slightly different handling and logging than the version | ||||||
| // in main.rs. | ||||||
| async fn upgrade_db(db: &sqlite::Connections) -> Result<()> { | ||||||
| db.conn(|conn| { | ||||||
| let backup_file = sqlite::backup_db(conn)?; | ||||||
| log::warn!("Old database backed up to {backup_file:?}"); | ||||||
| Ok(()) | ||||||
| }) | ||||||
| .await?; | ||||||
|
|
||||||
| let version = db | ||||||
| .write_tx(|tx| { | ||||||
| sqlite::migrate_schema(tx, db::MIGRATIONS) | ||||||
| .with_context(|| "Upgrading database schema failed") | ||||||
| }) | ||||||
| .await?; | ||||||
|
|
||||||
| log::warn!("Database upgraded to version {version}"); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion(non-blocking):
Suggested change
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| /// Controls the running application. | ||||||
| #[derive(Debug)] | ||||||
| pub struct RunControl { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| use anyhow::{Context, Result, anyhow, bail}; | ||
| use log::LevelFilter; | ||
| use mgmtd::config::LogTarget; | ||
| use mgmtd::config::{DbUpgrade, LogTarget}; | ||
| use mgmtd::db::{self}; | ||
| use mgmtd::license::LicenseVerifier; | ||
| use mgmtd::{StaticInfo, start}; | ||
|
|
@@ -16,6 +16,7 @@ use uuid::Uuid; | |
|
|
||
| fn main() -> Result<(), i32> { | ||
| inner_main().map_err(|err| { | ||
| log::error!("{err:#}"); | ||
| eprintln!("{err:#}"); | ||
| 1 | ||
| })?; | ||
|
|
@@ -72,7 +73,7 @@ fn inner_main() -> Result<()> { | |
| return Ok(()); | ||
| } | ||
|
|
||
| if user_config.upgrade { | ||
| if user_config.db_upgrade == DbUpgrade::True { | ||
| upgrade_db(&user_config.db_file)?; | ||
| return Ok(()); | ||
| } | ||
|
|
@@ -217,15 +218,16 @@ beegfs-mgmtd.conf. Before starting the management, you must MANUALLY transfer yo | |
| fn upgrade_db(db_file: &Path) -> Result<()> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion(non-blocking): it would be worth mentioning this only is used for the manual upgrade path and for automatic upgrades see the version in |
||
| let mut conn = sqlite::open(db_file)?; | ||
|
|
||
| let backup_file = sqlite::backup_db(&mut conn)?; | ||
| let tx = conn.transaction()?; | ||
|
|
||
| let backup_file = sqlite::backup_db(&tx)?; | ||
| println!("Old database backed up to {backup_file:?}"); | ||
|
|
||
| let tx = conn.transaction()?; | ||
| let version = sqlite::migrate_schema(&tx, db::MIGRATIONS) | ||
| .with_context(|| "Upgrading database schema failed")?; | ||
| tx.commit()?; | ||
|
|
||
| println!("Upgraded database to version {version}"); | ||
| println!("Database upgraded to version {version}"); | ||
| Ok(()) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(blocking): after thinking it over I think it would be more user friendly to just introduce a new boolean
db-auto-upgradeparameter. That way we can just keep the--upgradeflag as it is today which avoids needing to update docs, training, etc.