[pbs-devel] [PATCH v2 proxmox-backup 2/4] pbs-datastore: add check for maintenance to lookup_datastore

Dominik Csapak d.csapak at proxmox.com
Tue Oct 12 10:51:49 CEST 2021


much better, since now we

* do not accidentally lookup a datastore without making the intent clear
* and have the intent as parameter

though maybe it would have been even nicer to give the intended
operation (read/write/none) instead of the maintanence type directly,
so the user does not need to know which maintenance type does which?
(though it is very clear now too, so see this more as an optional 
comment...)

some more comments inline

On 10/6/21 17:14, Hannes Laimer wrote:
> lookup_datastore now takes a second parameter, an option
> 'acceptable_maintenance'-type.
> If None is passed, the datastore has to be in no maintenance at all.
> If read-only is passed, the datastore can either be in 'read-only' or
> any less restrictive maintenance type.
> If offline is passed, the datastore can either be in 'offline' or
> any less restrictive maintenance type.
> ---
>   pbs-datastore/src/datastore.rs       | 14 +++++++--
>   pbs-datastore/src/snapshot_reader.rs |  6 +++-
>   src/api2/admin/datastore.rs          | 44 ++++++++++++++--------------
>   src/api2/backup/mod.rs               |  2 +-
>   src/api2/pull.rs                     |  4 +--
>   src/api2/reader/mod.rs               |  6 ++--
>   src/api2/status.rs                   |  6 ++--
>   src/api2/tape/backup.rs              |  4 +--
>   src/api2/tape/restore.rs             |  6 ++--
>   src/bin/proxmox-backup-proxy.rs      |  6 ++--
>   src/server/prune_job.rs              |  2 +-
>   src/server/verify_job.rs             |  4 +--
>   12 files changed, 59 insertions(+), 45 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index b068a9c9..faaff1ca 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -11,7 +11,7 @@ use lazy_static::lazy_static;
>   
>   use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions};
>   
> -use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus};
> +use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus, MaintenanceType};
>   use pbs_tools::format::HumanByte;
>   use pbs_tools::fs::{lock_dir_noblock, DirLockGuard};
>   use pbs_tools::process_locker::ProcessLockSharedGuard;
> @@ -61,12 +61,22 @@ pub struct DataStore {
>   
>   impl DataStore {
>   
> -    pub fn lookup_datastore(name: &str) -> Result<Arc<DataStore>, Error> {
> +    pub fn lookup_datastore(name: &str, acceptable_maintenance: Option<MaintenanceType>) -> Result<Arc<DataStore>, Error> {
>   
>           let (config, _digest) = pbs_config::datastore::config()?;
>           let config: DataStoreConfig = config.lookup("datastore", name)?;
>           let path = PathBuf::from(&config.path);
>   
> +        if let Some(maintenance_type) = config.maintenance_type {
> +            if acceptable_maintenance.map_or(true, |acceptable| maintenance_type > acceptable) {
> +                bail!("Datastore '{}' is in maintenance '{}' mode: {}",
> +                    name,
> +                    maintenance_type,
> +                    config.maintenance_msg.unwrap_or(String::from("no description"))
> +                );
> +            }
> +        }

imho it would be better to write it as a match like this:

match (config.maintenance_type, acceptable_maintenance) => {
     (Some(a), Some(b)) if a > b | (Some(_), None) => error,
     (None, _) => {},
}

?

> +
>           let mut map = DATASTORE_MAP.lock().unwrap();
>   
>           if let Some(datastore) = map.get(name) {
> diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
> index c49cf16f..5b1f6a0f 100644
> --- a/pbs-datastore/src/snapshot_reader.rs
> +++ b/pbs-datastore/src/snapshot_reader.rs
> @@ -12,6 +12,7 @@ use crate::fixed_index::FixedIndexReader;
>   use crate::dynamic_index::DynamicIndexReader;
>   use crate::manifest::{archive_type, ArchiveType, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
>   use crate::DataStore;
> +use pbs_api_types::MaintenanceType;
>   use pbs_tools::fs::lock_dir_noblock_shared;
>   
>   /// Helper to access the contents of a datastore backup snapshot
> @@ -119,7 +120,10 @@ impl <'a> Iterator for SnapshotChunkIterator<'a> {
>                           };
>   
>                           let datastore =
> -                            DataStore::lookup_datastore(self.snapshot_reader.datastore_name())?;
> +                            DataStore::lookup_datastore(
> +                                self.snapshot_reader.datastore_name(),
> +                                Some(MaintenanceType::ReadOnly)
> +                            )?;
>                           let order = datastore.get_chunks_in_order(&index, |_| false, |_| Ok(()))?;
>   
>                           self.current_index = Some((Arc::new(index), 0, order));
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 7e9a0ee0..f97e3642 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -28,7 +28,7 @@ use pxar::EntryKind;
>   
>   use pbs_api_types::{ Authid, BackupContent, Counts, CryptMode,
>       DataStoreListItem, GarbageCollectionStatus, GroupListItem,
> -    SnapshotListItem, SnapshotVerifyState, PruneOptions,
> +    MaintenanceType, SnapshotListItem, SnapshotVerifyState, PruneOptions,
>       DataStoreStatus, RRDMode, RRDTimeFrameResolution,
>       BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA,
>       BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
> @@ -171,7 +171,7 @@ pub fn list_groups(
>       let user_info = CachedUserInfo::new()?;
>       let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>       let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
>   
>       let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
> @@ -269,7 +269,7 @@ pub fn delete_group(
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let group = BackupGroup::new(backup_type, backup_id);
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -314,7 +314,7 @@ pub fn list_snapshot_files(
>   ) -> Result<Vec<BackupContent>, Error> {
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
>   
> @@ -364,7 +364,7 @@ pub fn delete_snapshot(
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -413,7 +413,7 @@ pub fn list_snapshots (
>   
>       let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let base_path = datastore.base_path();
>   
> @@ -607,7 +607,7 @@ pub fn status(
>       _info: &ApiMethod,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<DataStoreStatus, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>       let storage = crate::tools::disks::disk_usage(&datastore.base_path())?;
>       let (counts, gc_status) = if verbose {
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -685,7 +685,7 @@ pub fn verify(
>       outdated_after: Option<i64>,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;

does the verify not write to the datastore (changes the verify state) ?
or is changing the manifest not considered 'writing to the datastore' ?
either way, we should maybe decide on this and comment it here?

>       let ignore_verified = ignore_verified.unwrap_or(true);
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -830,7 +830,7 @@ pub fn prune(
>   
>       let group = BackupGroup::new(&backup_type, &backup_id);
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -953,7 +953,7 @@ pub fn prune_datastore(
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>   
> @@ -997,7 +997,7 @@ pub fn start_garbage_collection(
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let job =  Job::new("garbage_collection", &store)
> @@ -1033,7 +1033,7 @@ pub fn garbage_collection_status(
>       _rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<GarbageCollectionStatus, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::Offline))?;
>   
>       let status = datastore.last_gc_status();
>   
> @@ -1109,7 +1109,7 @@ pub fn download_file(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1179,7 +1179,7 @@ pub fn download_file_decoded(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1293,7 +1293,7 @@ pub fn upload_backup_log(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, None)?;
>   
>           let file_name =  CLIENT_LOG_BLOB_NAME;
>   
> @@ -1370,7 +1370,7 @@ pub fn catalog(
>       filepath: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Vec<ArchiveEntry>, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1440,7 +1440,7 @@ pub fn pxar_file_download(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(&store)?;
> +        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1595,7 +1595,7 @@ pub fn get_group_notes(
>       backup_id: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
> @@ -1637,7 +1637,7 @@ pub fn set_group_notes(
>       notes: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
> @@ -1679,7 +1679,7 @@ pub fn get_notes(
>       backup_time: i64,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> @@ -1730,7 +1730,7 @@ pub fn set_notes(
>       notes: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;

imho if the verify state change does not count as a write, this 
shouldn't either (both modify the manifest)

>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> @@ -1775,7 +1775,7 @@ pub fn set_backup_owner(
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let backup_group = BackupGroup::new(backup_type, backup_id);
>   
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 4432c8d5..8e45ec1f 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -74,7 +74,7 @@ async move {
>       let user_info = CachedUserInfo::new()?;
>       user_info.check_privs(&auth_id, &["datastore", &store], PRIV_DATASTORE_BACKUP, false)?;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let backup_type = required_string_param(&param, "backup-type")?;
>       let backup_id = required_string_param(&param, "backup-id")?;
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index ea8faab8..26b8a3be 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -9,7 +9,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironment, Permission};
>   
>   use pbs_client::{HttpClient, BackupRepository};
>   use pbs_api_types::{
> -    Remote, Authid, SyncJobConfig,
> +    Remote, Authid, MaintenanceType, SyncJobConfig,
>       DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
>       PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
>   };
> @@ -46,7 +46,7 @@ pub async fn get_pull_parameters(
>       remote_store: &str,
>   ) -> Result<(HttpClient, BackupRepository, Arc<DataStore>), Error> {
>   
> -    let tgt_store = DataStore::lookup_datastore(store)?;
> +    let tgt_store = DataStore::lookup_datastore(store, Some(MaintenanceType::Offline))?;
>   
>       let (remote_config, _digest) = pbs_config::remote::config()?;
>       let remote: Remote = remote_config.lookup("remote", remote)?;
> diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
> index 1fd25a5c..f209e50a 100644
> --- a/src/api2/reader/mod.rs
> +++ b/src/api2/reader/mod.rs
> @@ -28,8 +28,8 @@ use proxmox::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_ID_SCHEMA,
> -    CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
> +    Authid, MaintenanceType, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA,
> +    BACKUP_ID_SCHEMA, CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
>       BACKUP_ARCHIVE_NAME_SCHEMA,
>   };
>   use pbs_tools::fs::lock_dir_noblock_shared;
> @@ -93,7 +93,7 @@ fn upgrade_to_backup_reader_protocol(
>               bail!("no permissions on /datastore/{}", store);
>           }
>   
> -        let datastore = DataStore::lookup_datastore(&store)?;
> +        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>           let backup_type = required_string_param(&param, "backup-type")?;
>           let backup_id = required_string_param(&param, "backup-id")?;
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index 548e5319..3f168f25 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -15,8 +15,8 @@ use proxmox::api::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, DATASTORE_SCHEMA, RRDMode, RRDTimeFrameResolution,
> -    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> +    Authid, MaintenanceType, DATASTORE_SCHEMA, RRDMode,
> +    RRDTimeFrameResolution, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
>   };
>   
>   use pbs_datastore::DataStore;
> @@ -98,7 +98,7 @@ pub fn datastore_status(
>               continue;
>           }
>   
> -        let datastore = match DataStore::lookup_datastore(&store) {
> +        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {
>               Ok(datastore) => datastore,
>               Err(err) => {
>                   list.push(json!({
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index 7627e35b..05ed9c53 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -174,7 +174,7 @@ pub fn do_tape_backup_job(
>   
>       let worker_type = job.jobtype().to_string();
>   
> -    let datastore = DataStore::lookup_datastore(&setup.store)?;
> +    let datastore = DataStore::lookup_datastore(&setup.store, None)?;

this and tape restore are reversed:

a tape backup job reads from the datastore and a tape restore job writes
to the datastore

>   
>       let (config, _digest) = pbs_config::media_pool::config()?;
>       let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
> @@ -355,7 +355,7 @@ pub fn backup(
>           &setup.drive,
>       )?;
>   
> -    let datastore = DataStore::lookup_datastore(&setup.store)?;
> +    let datastore = DataStore::lookup_datastore(&setup.store, None)?;

same here

>   
>       let (config, _digest) = pbs_config::media_pool::config()?;
>       let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 61e17d1b..a23c69c7 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -29,7 +29,7 @@ use proxmox::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, Userid, CryptMode,
> +    Authid, MaintenanceType, Userid, CryptMode,
>       DATASTORE_MAP_ARRAY_SCHEMA, DATASTORE_MAP_LIST_SCHEMA, DRIVE_NAME_SCHEMA,
>       UPID_SCHEMA, TAPE_RESTORE_SNAPSHOT_SCHEMA,
>       PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_TAPE_READ,
> @@ -106,10 +106,10 @@ impl TryFrom<String> for DataStoreMap {
>               if let Some(index) = store.find('=') {
>                   let mut target = store.split_off(index);
>                   target.remove(0); // remove '='
> -                let datastore = DataStore::lookup_datastore(&target)?;
> +                let datastore = DataStore::lookup_datastore(&target, Some(MaintenanceType::ReadOnly))?;

same here

>                   map.insert(store, datastore);
>               } else if default.is_none() {
> -                default = Some(DataStore::lookup_datastore(&store)?);
> +                default = Some(DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?);

same here

>               } else {
>                   bail!("multiple default stores given");
>               }
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 9199ebae..f80b9d7c 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -47,8 +47,8 @@ use proxmox_systemd::time::{compute_next_event, parse_calendar_event};
>   use pbs_tools::logrotate::LogRotate;
>   
>   use pbs_api_types::{
> -    Authid, TapeBackupJobConfig, VerificationJobConfig, SyncJobConfig, DataStoreConfig,
> -    PruneOptions,
> +    Authid, MaintenanceType, TapeBackupJobConfig, VerificationJobConfig,
> +    SyncJobConfig, DataStoreConfig, PruneOptions,
>   };
>   
>   use proxmox_rest_server::daemon;
> @@ -527,7 +527,7 @@ async fn schedule_datastore_garbage_collection() {
>       };
>   
>       for (store, (_, store_config)) in config.sections {
> -        let datastore = match DataStore::lookup_datastore(&store) {
> +        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {

in this special case, i'd even allow 'offline' for checking the schedule
iff the schedule triggers, we'd have to lookup the datastore again with 
the correct maintenance type (None, since it writes to the datstore, no?)

now it would log 'lookup_datastore failed - {err}' every minute while
the datastore is in offline maintenance and even do gc when it is in
read-only mode

if there is a reason why gc jobs should work in read-only mode,
it'd warrant a comment i think

>               Ok(datastore) => datastore,
>               Err(err) => {
>                   eprintln!("lookup_datastore failed - {}", err);
> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
> index fc6443e9..00a055f4 100644
> --- a/src/server/prune_job.rs
> +++ b/src/server/prune_job.rs
> @@ -96,7 +96,7 @@ pub fn do_prune_job(
>       auth_id: &Authid,
>       schedule: Option<String>,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let worker_type = job.jobtype().to_string();
>       let auth_id = auth_id.clone();
> diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
> index 6aba97c9..05c43602 100644
> --- a/src/server/verify_job.rs
> +++ b/src/server/verify_job.rs
> @@ -1,7 +1,7 @@
>   use anyhow::{format_err, Error};
>   
>   use pbs_tools::task_log;
> -use pbs_api_types::{Authid, VerificationJobConfig};
> +use pbs_api_types::{Authid, MaintenanceType, VerificationJobConfig};
>   use proxmox_rest_server::WorkerTask;
>   use pbs_datastore::DataStore;
>   
> @@ -22,7 +22,7 @@ pub fn do_verification_job(
>       to_stdout: bool,
>   ) -> Result<String, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&verification_job.store)?;
> +    let datastore = DataStore::lookup_datastore(&verification_job.store, Some(MaintenanceType::ReadOnly))?;

same reasoning as above, either writing to the manifest is a 'write' or 
the above code in 'set_notes' is wrong

>   
>       let outdated_after = verification_job.outdated_after;
>       let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
> 






More information about the pbs-devel mailing list