[pbs-devel] [PATCH proxmox-backup v2 1/2] fix #5439: allow to reuse existing datastore
Gabriel Goller
g.goller at proxmox.com
Fri Jul 12 13:57:34 CEST 2024
On 10.07.2024 15:28, Fabian Grünbichler wrote:
>On June 12, 2024 3:22 pm, Gabriel Goller wrote:
>> Disallow creating datastores in non-empty directories. Allow adding
>> existing datastores via a 'reuse-datastore' checkmark. This only checks
>> if all the necessary directories (.chunks + subdirectories and .lock)
>> are existing and have the correct permissions, then opens the
>> datastore.
>>
>> Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
>> ---
>>
>> v2, thanks @Fabian:
>> - also check on frontend for root
>> - forbid datastore creation if dir not empty
>> - add reuse-datastore option
>> - verify chunkstore directories permissions and owners
>>
>> pbs-datastore/src/chunk_store.rs | 49 ++++++++++++++++++++++++++++-
>> src/api2/config/datastore.rs | 54 +++++++++++++++++++++++++-------
>> src/api2/node/disks/directory.rs | 1 +
>> src/api2/node/disks/zfs.rs | 1 +
>> 4 files changed, 93 insertions(+), 12 deletions(-)
>>
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index 9f6289c9..077bc316 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -164,7 +164,7 @@ impl ChunkStore {
>> /// Note that this must be used with care, as it's dangerous to create two instances on the
>> /// same base path, as closing the underlying ProcessLocker drops all locks from this process
>> /// on the lockfile (even if separate FDs)
>
>given this ^
>
>> - pub(crate) fn open<P: Into<PathBuf>>(
>> + pub fn open<P: Into<PathBuf>>(
>
>wouldn't it make more sense to split out the path checks and call them
>both in open and in verify_chunkstore below
Why call them both in open and verify_chunkstore? create_datastore
always calls the verify method before open, so we could move this to
the verify function? I think I don't understand this correctly...
>> name: &str,
>> base: P,
>> sync_level: DatastoreFSyncLevel,
>> @@ -563,6 +563,53 @@ impl ChunkStore {
>> // unwrap: only `None` in unit tests
>> ProcessLocker::try_exclusive_lock(self.locker.clone().unwrap())
>> }
>> +
>> + /// Checks permissions and owner of passed path.
>> + fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
>> + match nix::sys::stat::stat(path.as_ref()) {
>> + Ok(stat) => {
>> + if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
>> + || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
>> + || stat.st_mode & 0o700 != file_mode
>> + {
>> + bail!(
>> + "unable to open existing chunk store path {:?} - permissions or owner not correct",
>> + path.as_ref(),
>> + );
>> + }
>> + }
>> + Err(err) => {
>> + bail!(
>> + "unable to open existing chunk store path {:?} - {err}",
>> + path.as_ref(),
>> + );
>> + }
>> + }
>> + Ok(())
>> + }
>> +
>> + /// Verify vital files in datastore. Checks the owner and permissions of: the chunkstore, it's
>> + /// subdirectories and the lock file.
>> + pub fn verify_chunkstore<T: AsRef<Path>>(path: T) -> Result<(), Error> {
>> + // Check datastore root path perm/owner
>> + ChunkStore::check_permissions(path.as_ref(), 0o700)?;
>> +
>> + let chunk_dir = Self::chunk_dir(path.as_ref());
>> + // Check datastore .chunks path perm/owner
>> + ChunkStore::check_permissions(&chunk_dir, 0o700)?;
>> +
>> + // Check all .chunks subdirectories
>> + for i in 0..64 * 1024 {
>> + let mut l1path = chunk_dir.clone();
>> + l1path.push(format!("{:04x}", i));
>> + ChunkStore::check_permissions(&l1path, 0o700)?;
>> + }
>> +
>> + // Check .lock file
>> + let lockfile_path = Self::lockfile_path(path.as_ref());
>> + ChunkStore::check_permissions(lockfile_path, 0o600)?;
>> + Ok(())
>> + }
>> }
>>
>> #[test]
>> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
>> index 6b742acb..f1e80b2d 100644
>> --- a/src/api2/config/datastore.rs
>> +++ b/src/api2/config/datastore.rs
>> @@ -1,7 +1,7 @@
>> use std::path::PathBuf;
>>
>> use ::serde::{Deserialize, Serialize};
>> -use anyhow::Error;
>> +use anyhow::{bail, Error};
>> use hex::FromHex;
>> use serde_json::Value;
>>
>> @@ -70,23 +70,48 @@ pub(crate) fn do_create_datastore(
>> _lock: BackupLockGuard,
>> mut config: SectionConfigData,
>> datastore: DataStoreConfig,
>> + reuse_datastore: bool,
>> worker: Option<&dyn WorkerTaskContext>,
>> ) -> Result<(), Error> {
>> let path: PathBuf = datastore.path.clone().into();
>>
>> + if path.parent().is_none() {
>> + bail!("cannot create datastore in root path");
>> + }
>> +
>> + let datastore_empty = std::fs::read_dir(path.clone())?.all(|dir| {
>
>doesn't this read_dir fail if `path` doesn't exist yet? prior to this
>change, creating a datastore would also create the datastore dir itself
>(and any parents!).
Yes, correct. Changed it to another map_or.
>> + dir.map_or(false, |file| {
>> + file.file_name().to_str().map_or(false, |name| {
>> + (name.starts_with('.')
>> + && !(name == ".chunks" || name == ".lock" || name == ".gc-status"))
>> + || name.starts_with("lost+found")
>
>I think the last check there could be == as well?
Yep.
>the indentation here is kinda whacky.. maybe it would be enough to
>consider `.chunks` to mark a dir as non-empty?
Hmm, yeah this would make everything simpler.
>> + })
>> + })
>>+ });
>
>should we do this always? or restructure the ifs below and just do it
>for the non-reusing create path?
Oh, yeah that would work. We already check the existance of `.chunks`
with the verify method (though implicitly) so that should work.
>> +
>> let tuning: DatastoreTuning = serde_json::from_value(
>> DatastoreTuning::API_SCHEMA
>> .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
>> )?;
>> - let backup_user = pbs_config::backup_user()?;
>> - let _store = ChunkStore::create(
>> - &datastore.name,
>> - path,
>> - backup_user.uid,
>> - backup_user.gid,
>> - worker,
>> - tuning.sync_level.unwrap_or_default(),
>> - )?;
>> +
>> + if !datastore_empty && !reuse_datastore {
>> + bail!("Directory is not empty!");
>> + } else if !datastore_empty && reuse_datastore {
>> + ChunkStore::verify_chunkstore(&path)?;
>> +
>> + let _store =
>> + ChunkStore::open(&datastore.name, path, tuning.sync_level.unwrap_or_default())?;
>
>(continued from first comment above) and leave out this call here?
>
>> + } else {
>> + let backup_user = pbs_config::backup_user()?;
>> + let _store = ChunkStore::create(
>> + &datastore.name,
>> + path,
>> + backup_user.uid,
>> + backup_user.gid,
>> + worker,
>> + tuning.sync_level.unwrap_or_default(),
>> + )?;
>> + }
>>
>> config.set_data(&datastore.name, "datastore", &datastore)?;
>>
>> @@ -103,6 +128,12 @@ pub(crate) fn do_create_datastore(
>> type: DataStoreConfig,
>> flatten: true,
>> },
>> + "reuse-datastore": {
>> + type: Boolean,
>> + optional: true,
>> + default: false,
>> + description: "Re-use existing datastore directory."
>> + }
>> },
>> },
>> access: {
>> @@ -112,6 +143,7 @@ pub(crate) fn do_create_datastore(
>> /// Create new datastore config.
>> pub fn create_datastore(
>> config: DataStoreConfig,
>> + reuse_datastore: bool,
>> rpcenv: &mut dyn RpcEnvironment,
>> ) -> Result<String, Error> {
>> let lock = pbs_config::datastore::lock_config()?;
>> @@ -156,7 +188,7 @@ pub fn create_datastore(
>> auth_id.to_string(),
>> to_stdout,
>> move |worker| {
>> - do_create_datastore(lock, section_config, config, Some(&worker))?;
>> + do_create_datastore(lock, section_config, config, reuse_datastore, Some(&worker))?;
>>
>> if let Some(prune_job_config) = prune_job_config {
>> do_create_prune_job(prune_job_config, Some(&worker))
>> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
>> index 9f1112a9..25cfe784 100644
>> --- a/src/api2/node/disks/directory.rs
>> +++ b/src/api2/node/disks/directory.rs
>> @@ -217,6 +217,7 @@ pub fn create_datastore_disk(
>> lock,
>> config,
>> datastore,
>> + false,
>> Some(&worker),
>> )?;
>> }
>> diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
>> index 673dc1bf..ad2129fc 100644
>> --- a/src/api2/node/disks/zfs.rs
>> +++ b/src/api2/node/disks/zfs.rs
>> @@ -323,6 +323,7 @@ pub fn create_zpool(
>> lock,
>> config,
>> datastore,
>> + false,
>> Some(&worker),
>> )?;
>> }
>> --
>> 2.43.0
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
>
>
>_______________________________________________
>pbs-devel mailing list
>pbs-devel at lists.proxmox.com
>https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
More information about the pbs-devel
mailing list