[pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes

[Dominik Csapak]

On 1/31/25 17:10, Laurențiu Leahu-Vlăducu wrote:
> Thanks for your review! I already made almost all the requested changes. Some further comments inline:
> 

[snip]>> On 1/16/25 12:51, Laurențiu Leahu-Vlăducu wrote:
[snip]
>>> +                    for index in 0..idle_drives.len() {
>>> +                        let existing_drive = 
>>> drives_info.drives.get(&idle_drives[index].config.name);
>>> +
>>> +                        match existing_drive {
>>> +                            Some(existing_drive) => {
>>> +                                if oldest_time.is_none() || oldest_time.is_some_and(| 
>>> oldest_time| existing_drive.last_usage < oldest_time) {
>>> +                                    oldest_time = Option::from(existing_drive.last_usage);
>>> +                                    index_oldest = Option::from(index);
>>> +                                }
>>> +                            },
>>> +                            None => {
>>> +                                // Drive has never been used, so let's use this one!
>>> +                                index_oldest = Option::from(index);
>>> +                                break;
>>> +                            }
>>> +                        }
>>> +                    }
>>> +
>>> +                    match index_oldest {
>>> +                        Some(index_oldest) => 
>>> Ok(idle_drives.get(index_oldest).unwrap().config.name.clone()),
>>> +                        None => bail!("there are no idle drives to choose for automatic drive 
>>> assignment")
>>> +                    }
>>> +                },
>>> +                Err(_error) => {
>>> +                    // Simply use the first drive, if possible.
>>> +                    match idle_drives.first() {
>>> +                        Some(idle_drive) => Ok(idle_drive.config.name.clone()),
>>> +                        None => bail!("there are no idle drives to choose for automatic drive 
>>> assignment")
>>> +                    }
>>> +                }
>>
>> wouldn't the code here get much easier if the 'drives_info' would return a list sorted by the 
>> last_usage?
>>
>> the i think the code could boil down to something like this (just pseudocode):
>>
>> let idle_drives = get_idle_drives();
>> let drives_in_order = get_sorted_drives_info(); // should include all drives, also those that have 
>> no last usage recorded (sorted before ones with usage)
>>
>> let chosen = drives_in_order.filter(|drive| idle_drives.contains(drive)).first();
>>
>> match chosen {
>>     Some(xx) => // do something with chosen drive and update list
>>     None => // fallback mechanism or error
>> }
>>
> 
> Sure, this would make it simpler for this use case - I agree. Taking a step back for a moment, we do 
> not have any files storing any state for tape drives/changers except the ones made to be changed by 
> the user (in /etc/proxmox-backup/tape.cfg).
> 
> My first thought was to create such a file for storing states related to tape drives/changers where 
> we might want to store more than the time of the last usage (although only this is stored, at the 
> moment). This is the reason why I named it "drives_info" or "tape_drive_info". In that context, 
> sorting drives by the last time of usage can be added, but is pretty specific.
> 
> However, if a more general file for such purposes is not desired, I can gladly make it more use 
> case-specific and rename the file and structs altogether to better reflect what the new code does. 
> Returning a sorted list of drives would make more sense in that context.

IMO the file name + state structure can still be generic. What I meant here was the internal
interface to get the list of drives. That does not even have to be coupled with the underlying
state in the file. (it just makes it easier).

E.g. could be enough if 'DrivesInfo' (or however it'll be named) has a 'get_sorted_by_usage' method
that returns a sorted list

[snip]
>>> diff --git a/src/tape/drive_info.rs b/src/tape/drive_info.rs
>>> new file mode 100644
>>> index 00000000..a0c2f836
>>> --- /dev/null
>>> +++ b/src/tape/drive_info.rs
>>> @@ -0,0 +1,51 @@
>>> +//! Serialize/deserialize tpae drive info (e.g. useful for statistics)
>>> +//!
>>> +//! This can be used to store a state over a longer period of time (e.g. last tape drive usage).
>>> +
>>> +use serde::{Serialize, Deserialize};
>>> +use std::collections::HashMap;
>>> +use anyhow::Error;
>>> +use proxmox_product_config::ApiLockGuard;
>>> +
>>> +/// Drive info file name
>>> +pub const DRIVE_INFO_FILENAME: &str = concat! (pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR_M!(), "/ 
>>> tape_drive_info.json");
>>> +/// Lock file name (used to prevent concurrent access)
>>> +pub const DRIVE_INFO_LOCKFILE: &str = concat! (pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR_M!(), 
>>> "/.tape_drive_info.json.lock");
>>> +
>>> +#[derive(Serialize, Deserialize, Default)]
>>> +pub struct SingleDriveInfo {
>>> +    #[serde(with = "proxmox_serde::epoch_as_rfc3339")]
>>> +    pub last_usage: i64,
>>> +}
>>> +
>>> +#[derive(Serialize, Deserialize, Default)]
>>> +pub struct DrivesInfo {
>>> +    pub drives: HashMap<String, SingleDriveInfo>,
>>> +}
>>
>> as said above, this would probably more economic if it were just a list
>>
>> maybe a `Vec<(String, i64)>` sorted by the i64
>>
>> also not a super fan of the name `DrivesInfo` or `SingleDriveInfo` since it does not really
>> convey what it does. (E.g. I'd have expected it to have much more info than just
>> the last usage)
>>
>> Maybe call it something like 'DrivesLastUsed' for now, and if we see we can
>> add more information, we can still update the name.
>> (Though I'm not the best at naming, so maybe someone else wants to chime in on this)
>>
> 
> I thought I could keep the file and name rather general, as we might add more to it in the future - 
> see my comment above. Of course, I can change it, if desired :)
> 

Similar point as above. IMO the file name can be generic, just the internal name of the struct
should be more reflective of what it does (maybe adding a comment to the filename that
we might extend the information saved would be helpful)

changing file names after the fact is "hard", so I'd want to avoid that too, but
an internal interface (struct name, method signature) is relatively easy to change,
even more so when it's not a separate published crate. And since the internal name is what
developers most often see (not necessarily the filename) it's important to convey
the correct content. That makes reading and using it much easier.