public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup] Automatically select a drive (if part of a changer) when loading tapes
@ 2025-01-16 11:51 Laurențiu Leahu-Vlăducu
  2025-01-16 11:51 ` [pbs-devel] [PATCH proxmox-backup 1/1] " Laurențiu Leahu-Vlăducu
  0 siblings, 1 reply; 6+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-16 11:51 UTC (permalink / raw)
  To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu

This patch adds the possibility to load tapes into drives automatically
by specifying a changer. This is useful for larger tape libraries.
Choosing a drive is done by using the drive that has not been used the
longest.

At the moment, this patch implements the functionality for automatic
loading both over the API and using the proxmox-backup CLI tool, as well
as in the web UI when selecting a changer. A second patch will add the
same functionality when configuring backup jobs.

Partially fixes #3351


Laurențiu Leahu-Vlăducu (1):
  Automatically select a drive (if part of a changer) when loading tapes

 Cargo.toml                        |   2 +
 pbs-api-types/src/tape/changer.rs |  14 +++-
 src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
 src/bin/proxmox-backup-api.rs     |   2 +
 src/bin/proxmox-backup-proxy.rs   |   2 +
 src/bin/proxmox-tape.rs           |  50 +++++++++++--
 src/tape/changer/mod.rs           |  19 ++++-
 src/tape/drive/virtual_tape.rs    |   8 +-
 src/tape/drive_info.rs            |  51 +++++++++++++
 src/tape/mod.rs                   |   1 +
 src/tools/mod.rs                  |   7 ++
 www/tape/ChangerStatus.js         |  16 +++-
 12 files changed, 274 insertions(+), 15 deletions(-)
 create mode 100644 src/tape/drive_info.rs

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes
  2025-01-16 11:51 [pbs-devel] [PATCH proxmox-backup] Automatically select a drive (if part of a changer) when loading tapes Laurențiu Leahu-Vlăducu
@ 2025-01-16 11:51 ` Laurențiu Leahu-Vlăducu
  2025-01-28 12:25   ` Dominik Csapak
  0 siblings, 1 reply; 6+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-16 11:51 UTC (permalink / raw)
  To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu

Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
---
 Cargo.toml                        |   2 +
 pbs-api-types/src/tape/changer.rs |  14 +++-
 src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
 src/bin/proxmox-backup-api.rs     |   2 +
 src/bin/proxmox-backup-proxy.rs   |   2 +
 src/bin/proxmox-tape.rs           |  50 +++++++++++--
 src/tape/changer/mod.rs           |  19 ++++-
 src/tape/drive/virtual_tape.rs    |   8 +-
 src/tape/drive_info.rs            |  51 +++++++++++++
 src/tape/mod.rs                   |   1 +
 src/tools/mod.rs                  |   7 ++
 www/tape/ChangerStatus.js         |  16 +++-
 12 files changed, 274 insertions(+), 15 deletions(-)
 create mode 100644 src/tape/drive_info.rs

diff --git a/Cargo.toml b/Cargo.toml
index adeeb6ef..348d1cea 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -72,6 +72,7 @@ proxmox-ldap = "0.2.1"
 proxmox-metrics = "0.3.1"
 proxmox-notify = "0.5.1"
 proxmox-openid = "0.10.0"
+proxmox-product-config = "0.2.2"
 proxmox-rest-server = { version = "0.8.5", features = [ "templates" ] }
 # some use "cli", some use "cli" and "server", pbs-config uses nothing
 proxmox-router = { version = "3.0.0", default-features = false }
@@ -221,6 +222,7 @@ proxmox-ldap.workspace = true
 proxmox-metrics.workspace = true
 proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
 proxmox-openid.workspace = true
+proxmox-product-config.workspace = true
 proxmox-rest-server = { workspace = true, features = [ "rate-limited-stream" ] }
 proxmox-router = { workspace = true, features = [ "cli", "server"] }
 proxmox-schema = { workspace = true, features = [ "api-macro" ] }
diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
index df3823cf..52ee08db 100644
--- a/pbs-api-types/src/tape/changer.rs
+++ b/pbs-api-types/src/tape/changer.rs
@@ -8,10 +8,20 @@ use proxmox_schema::{
 
 use crate::{OptionalDeviceIdentification, PROXMOX_SAFE_ID_FORMAT};
 
+const TAPE_CHANGER_MIN_LENGTH: usize = 3;
+const TAPE_CHANGER_MAX_LENGTH: usize = 32;
+
 pub const CHANGER_NAME_SCHEMA: Schema = StringSchema::new("Tape Changer Identifier.")
     .format(&PROXMOX_SAFE_ID_FORMAT)
-    .min_length(3)
-    .max_length(32)
+    .min_length(TAPE_CHANGER_MIN_LENGTH)
+    .max_length(TAPE_CHANGER_MAX_LENGTH)
+    .schema();
+
+pub const CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT: Schema =
+    StringSchema::new("Tape Changer Identifier to be used for automatic tape drive assignment.")
+    .format(&PROXMOX_SAFE_ID_FORMAT)
+    .min_length(TAPE_CHANGER_MIN_LENGTH)
+    .max_length(TAPE_CHANGER_MAX_LENGTH)
     .schema();
 
 pub const SCSI_CHANGER_PATH_SCHEMA: Schema =
diff --git a/src/api2/tape/changer.rs b/src/api2/tape/changer.rs
index 7ecf7bff..b73d6832 100644
--- a/src/api2/tape/changer.rs
+++ b/src/api2/tape/changer.rs
@@ -1,6 +1,6 @@
 use std::collections::HashMap;
 
-use anyhow::Error;
+use anyhow::{bail, Error};
 use serde_json::Value;
 
 use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
@@ -8,7 +8,8 @@ use proxmox_schema::api;
 
 use pbs_api_types::{
     Authid, ChangerListEntry, LtoTapeDrive, MtxEntryKind, MtxStatusEntry, ScsiTapeChanger,
-    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ,
+    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ, MEDIA_LABEL_SCHEMA, UPID_SCHEMA,
+    DriveListEntry, DeviceActivity
 };
 use pbs_config::CachedUserInfo;
 use pbs_tape::{
@@ -199,7 +200,119 @@ pub fn list_changers(
     Ok(list)
 }
 
+#[api(
+    input: {
+        properties: {
+            name: {
+                schema: CHANGER_NAME_SCHEMA,
+            },
+            "label-text": {
+                schema: MEDIA_LABEL_SCHEMA,
+            },
+        },
+    },
+    returns: {
+        schema: UPID_SCHEMA,
+    },
+    access: {
+        permission: &Permission::Privilege(&["tape", "device", "{name}"], PRIV_TAPE_READ, false),
+    },
+)]
+/// Load media with specified label
+///
+/// Issue a media load request to the associated changer device.
+pub fn load_media(
+    name: String,
+    label_text: String,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
+    let drive = choose_drive(&name, rpcenv);
+    super::drive::load_media(drive?, label_text, rpcenv)
+}
+
+#[api(
+    input: {
+        properties: {
+            name: {
+                schema: CHANGER_NAME_SCHEMA,
+            },
+            "source-slot": {
+                description: "Source slot number.",
+                minimum: 1,
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["tape", "device", "{name}"], PRIV_TAPE_READ, false),
+    },
+)]
+/// Load media from the specified slot
+///
+/// Issue a media load request to the associated changer device.
+pub async fn load_slot(
+    name: String,
+    source_slot: u64,
+    rpcenv: &mut dyn RpcEnvironment,) -> Result<(), Error> {
+    let drive = choose_drive(&name, rpcenv);
+    super::drive::load_slot(drive?, source_slot).await
+}
+
+fn choose_drive(
+    changer: &str,
+    rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
+    // We have to automatically select a drive from the specified changer. For that purpose, we take all drives that are part of the changer
+    // and search for the one that has not been used for the longest time (to ensure similar wear between the drives), or use one that has never been used.
+    let drives = super::drive::list_drives(Option::from(changer.to_string()), true, Value::Null, rpcenv);
+
+    match drives {
+        Ok(entries) => {
+            let idle_drives: Vec<DriveListEntry> = entries.into_iter().filter(|entry| matches!(entry.activity, None | Some(DeviceActivity::NoActivity))).collect();
+            let drives_info = crate::tape::drive_info::get_drives_info();
+
+            match drives_info {
+                Ok(drives_info) => {
+                    let mut index_oldest : Option<usize> = Option::default();
+                    let mut oldest_time: Option<i64> = Option::default();
+
+                    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")
+                    }
+                }
+            }
+        }
+        Err(error) => bail!("cannot query drives: {}", error)
+    }
+}
+
 const SUBDIRS: SubdirMap = &[
+    ("load-media", &Router::new().post(&API_METHOD_LOAD_MEDIA)),
+    ("load-slot", &Router::new().post(&API_METHOD_LOAD_SLOT)),
     ("status", &Router::new().get(&API_METHOD_GET_STATUS)),
     ("transfer", &Router::new().post(&API_METHOD_TRANSFER)),
 ];
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index 7a72d49a..c6b2d532 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -43,6 +43,8 @@ fn get_index() -> Pin<Box<dyn Future<Output = Response<Body>> + Send>> {
 async fn run() -> Result<(), Error> {
     init_logger("PBS_LOG", LevelFilter::INFO)?;
 
+    let _ = proxmox_backup::tools::init_product_config();
+
     config::create_configdir()?;
 
     config::update_self_signed_cert(false)?;
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index ce1be1c0..eb7da0e4 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -181,6 +181,8 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
 async fn run() -> Result<(), Error> {
     init_logger("PBS_LOG", LevelFilter::INFO)?;
 
+    let _ = proxmox_backup::tools::init_product_config();
+
     proxmox_backup::auth_helpers::setup_auth_context(false);
     proxmox_backup::server::notifications::init()?;
     metric_collection::init()?;
diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
index 8e8584b3..8de94af0 100644
--- a/src/bin/proxmox-tape.rs
+++ b/src/bin/proxmox-tape.rs
@@ -1,6 +1,8 @@
 use std::collections::HashMap;
 
 use anyhow::{bail, format_err, Error};
+use pbs_api_types::CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT;
+use pbs_config::drive::complete_changer_name;
 use serde_json::{json, Value};
 
 use proxmox_human_byte::HumanByte;
@@ -214,6 +216,10 @@ async fn eject_media(mut param: Value) -> Result<(), Error> {
                 schema: DRIVE_NAME_SCHEMA,
                 optional: true,
             },
+            changer: {
+                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
+                optional: true,
+            },
             "label-text": {
                 schema: MEDIA_LABEL_SCHEMA,
             },
@@ -230,11 +236,25 @@ async fn load_media(mut param: Value) -> Result<(), Error> {
 
     let (config, _digest) = pbs_config::drive::config()?;
 
-    let drive = extract_drive_name(&mut param, &config)?;
+    let drive = extract_drive_name(&mut param, &config);
+    let changer = param["changer"].as_str();
+
+    let path = match changer {
+        Some(changer) => {
+            format!("api2/json/tape/changer/{}/load-media", changer)
+        }
+        None => {
+            let drive = drive?;
+            format!("api2/json/tape/drive/{}/load-media", drive)
+        }
+    };
+
+    if let Some(param) = param.as_object_mut() {
+        param.remove("changer");
+    }
 
     let client = connect_to_localhost()?;
 
-    let path = format!("api2/json/tape/drive/{}/load-media", drive);
     let result = client.post(&path, Some(param)).await?;
 
     view_task_result(&client, result, &output_format).await?;
@@ -276,6 +296,10 @@ async fn export_media(mut param: Value) -> Result<(), Error> {
                 schema: DRIVE_NAME_SCHEMA,
                 optional: true,
             },
+            changer: {
+                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
+                optional: true,
+            },
             "source-slot": {
                 description: "Source slot number.",
                 type: u64,
@@ -288,11 +312,25 @@ async fn export_media(mut param: Value) -> Result<(), Error> {
 async fn load_media_from_slot(mut param: Value) -> Result<(), Error> {
     let (config, _digest) = pbs_config::drive::config()?;
 
-    let drive = extract_drive_name(&mut param, &config)?;
+    let drive = extract_drive_name(&mut param, &config);
+    let changer = param["changer"].as_str();
+
+    let path = match changer {
+        Some(changer) => {
+            format!("api2/json/tape/changer/{}/load-slot", changer)
+        }
+        None => {
+            let drive = drive?;
+            format!("api2/json/tape/drive/{}/load-slot", drive)
+        }
+    };
+
+    if let Some(param) = param.as_object_mut() {
+        param.remove("changer");
+    }
 
     let client = connect_to_localhost()?;
 
-    let path = format!("api2/json/tape/drive/{}/load-slot", drive);
     client.post(&path, Some(param)).await?;
 
     Ok(())
@@ -1091,13 +1129,15 @@ fn main() {
             CliCommand::new(&API_METHOD_LOAD_MEDIA)
                 .arg_param(&["label-text"])
                 .completion_cb("drive", complete_drive_name)
+                .completion_cb("changer", complete_changer_name)
                 .completion_cb("label-text", complete_media_label_text),
         )
         .insert(
             "load-media-from-slot",
             CliCommand::new(&API_METHOD_LOAD_MEDIA_FROM_SLOT)
                 .arg_param(&["source-slot"])
-                .completion_cb("drive", complete_drive_name),
+                .completion_cb("drive", complete_drive_name)
+                .completion_cb("changer", complete_changer_name),
         )
         .insert(
             "unload",
diff --git a/src/tape/changer/mod.rs b/src/tape/changer/mod.rs
index 18ea0f46..8e3ff0f7 100644
--- a/src/tape/changer/mod.rs
+++ b/src/tape/changer/mod.rs
@@ -273,6 +273,17 @@ pub trait MediaChange {
     }
 }
 
+/// Updates the drive's last usage time to now.
+pub(super) fn update_drive_usage(drive: &str) -> Result<(), Error> {
+    let _lock = crate::tape::drive_info::lock()?;
+
+    let mut drives_info = crate::tape::drive_info::get_drives_info()?;
+
+    let now = proxmox_time::epoch_i64();
+    drives_info.drives.entry(drive.into()).or_default().last_usage = now;
+    crate::tape::drive_info::save_config(&drives_info)
+}
+
 const USE_MTX: bool = false;
 
 impl ScsiMediaChange for ScsiTapeChanger {
@@ -423,7 +434,13 @@ impl MediaChange for MtxMediaChanger {
     }
 
     fn load_media_from_slot(&mut self, slot: u64) -> Result<MtxStatus, Error> {
-        self.config.load_slot(slot, self.drive_number())
+        let status = self.config.load_slot(slot, self.drive_number())?;
+
+        if let Err(err) = update_drive_usage(self.drive_name()) {
+            log::warn!("could not update drive usage: {err}");
+        }
+
+        Ok(status)
     }
 
     fn unload_media(&mut self, target_slot: Option<u64>) -> Result<MtxStatus, Error> {
diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index 866e4d32..213f17fe 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -567,7 +567,13 @@ impl MediaChange for VirtualTapeHandle {
         };
         self.store_status(&status)?;
 
-        self.status()
+        let status = self.status()?;
+
+        if let Err(err) = crate::tape::changer::update_drive_usage(self.drive_name()) {
+            log::warn!("could not update drive usage: {err}");
+        }
+
+        Ok(status)
     }
 
     fn unload_media(&mut self, _target_slot: Option<u64>) -> Result<MtxStatus, Error> {
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>,
+}
+
+/// Get exclusive lock
+pub fn lock() -> Result<ApiLockGuard, Error> {
+    proxmox_product_config::open_api_lockfile(DRIVE_INFO_LOCKFILE, Option::None, true)
+}
+
+/// Read and parse the drive info file
+pub fn get_drives_info() -> Result<DrivesInfo, Error> {
+    let content =
+        proxmox_sys::fs::file_read_optional_string(DRIVE_INFO_FILENAME)?;
+
+    match content {
+        Some(content) => {
+            let result = serde_json::from_str::<DrivesInfo>(&content)?;
+            Ok(result)
+        },
+        None => {
+            Ok(DrivesInfo::default())
+        }
+    }
+}
+
+/// Save the configuration file
+pub fn save_config(data: &DrivesInfo) -> Result<(), Error> {
+    let json = serde_json::to_string(data)?;
+    proxmox_product_config::replace_config(DRIVE_INFO_FILENAME, json.as_bytes())
+}
diff --git a/src/tape/mod.rs b/src/tape/mod.rs
index f276f948..8b87152d 100644
--- a/src/tape/mod.rs
+++ b/src/tape/mod.rs
@@ -20,6 +20,7 @@ pub use inventory::*;
 
 pub mod changer;
 pub mod drive;
+pub mod drive_info;
 pub mod encryption_keys;
 
 mod media_pool;
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index 322894dd..11fb2821 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -61,3 +61,10 @@ pub fn setup_safe_path_env() {
         std::env::remove_var(name);
     }
 }
+
+pub fn init_product_config() -> Result<(), Error> {
+    let backup_user = pbs_config::backup_user()?;
+    let root_user = nix::unistd::User::from_uid(nix::unistd::ROOT)?.expect("could not find root user");
+    proxmox_product_config::init(backup_user, root_user);
+    Ok(())
+}
diff --git a/www/tape/ChangerStatus.js b/www/tape/ChangerStatus.js
index e18af90e..0a875779 100644
--- a/www/tape/ChangerStatus.js
+++ b/www/tape/ChangerStatus.js
@@ -222,12 +222,17 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
 		    autoShow: true,
 		    submitText: gettext('OK'),
 		    title: gettext('Load Media into Drive'),
-		    url: `/api2/extjs/tape/drive`,
+		    url: `/api2/extjs/tape`,
 		    method: 'POST',
 		    submitUrl: function(url, values) {
-			let drive = values.drive;
-			delete values.drive;
-			return `${url}/${encodeURIComponent(drive)}/${apiCall}`;
+			    let drive = values.drive;
+			    delete values.drive;
+			    
+			    if (drive) {
+				    return `${url}/drive/${encodeURIComponent(drive)}/${apiCall}`;
+			    } else {
+				    return `${url}/changer/${encodeURIComponent(changer)}/${apiCall}`;
+			    }
 		    },
 		    items: [
 			label !== "" ? {
@@ -248,6 +253,9 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
 			    fieldLabel: gettext('Drive'),
 			    changer: changer,
 			    name: 'drive',
+			    emptyText: gettext('Choose Automatically'),
+			    allowBlank: true,
+			    autoSelect: false,
 			},
 		    ],
 		    listeners: {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes
  2025-01-16 11:51 ` [pbs-devel] [PATCH proxmox-backup 1/1] " Laurențiu Leahu-Vlăducu
@ 2025-01-28 12:25   ` Dominik Csapak
  2025-01-31 16:10     ` Laurențiu Leahu-Vlăducu
  0 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2025-01-28 12:25 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion,
	Laurențiu Leahu-Vlăducu

Thanks for tackling this!

a few high level comments:

* please write the relevant text as commit message (as mentioned off-list)
   basically the text in the (here not needed) cover-letter would fit
   as the commit message

* rustfmt: please use it, there are some lines that are very long.
   comments should probably also not exceed the line length limit

* it would be good to explain the reason for the code a bit more, e.g. you introduce
   the product-config crate here and do an initialization,
   but neither by looking at the code, nor by reading the commit
   message i know exactly why (i know it because we talked off-list)
   a short sentence like:

   added the product-config crate so we're able to reuse the
   'replace_config' instead of copying it.

   can already be enough

* the patch does not currently apply cleanly because we did
   some code moves (e.g. pbs-api-types is now an external crate)


Some other comments inline:

On 1/16/25 12:51, Laurențiu Leahu-Vlăducu wrote:
> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> ---
>   Cargo.toml                        |   2 +
>   pbs-api-types/src/tape/changer.rs |  14 +++-
>   src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
>   src/bin/proxmox-backup-api.rs     |   2 +
>   src/bin/proxmox-backup-proxy.rs   |   2 +
>   src/bin/proxmox-tape.rs           |  50 +++++++++++--
>   src/tape/changer/mod.rs           |  19 ++++-
>   src/tape/drive/virtual_tape.rs    |   8 +-
>   src/tape/drive_info.rs            |  51 +++++++++++++
>   src/tape/mod.rs                   |   1 +
>   src/tools/mod.rs                  |   7 ++
>   www/tape/ChangerStatus.js         |  16 +++-
>   12 files changed, 274 insertions(+), 15 deletions(-)
>   create mode 100644 src/tape/drive_info.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index adeeb6ef..348d1cea 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -72,6 +72,7 @@ proxmox-ldap = "0.2.1"
>   proxmox-metrics = "0.3.1"
>   proxmox-notify = "0.5.1"
>   proxmox-openid = "0.10.0"
> +proxmox-product-config = "0.2.2"
>   proxmox-rest-server = { version = "0.8.5", features = [ "templates" ] }
>   # some use "cli", some use "cli" and "server", pbs-config uses nothing
>   proxmox-router = { version = "3.0.0", default-features = false }
> @@ -221,6 +222,7 @@ proxmox-ldap.workspace = true
>   proxmox-metrics.workspace = true
>   proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
>   proxmox-openid.workspace = true
> +proxmox-product-config.workspace = true
>   proxmox-rest-server = { workspace = true, features = [ "rate-limited-stream" ] }
>   proxmox-router = { workspace = true, features = [ "cli", "server"] }
>   proxmox-schema = { workspace = true, features = [ "api-macro" ] }
> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
> index df3823cf..52ee08db 100644
> --- a/pbs-api-types/src/tape/changer.rs
> +++ b/pbs-api-types/src/tape/changer.rs
> @@ -8,10 +8,20 @@ use proxmox_schema::{
>   
>   use crate::{OptionalDeviceIdentification, PROXMOX_SAFE_ID_FORMAT};
>   
> +const TAPE_CHANGER_MIN_LENGTH: usize = 3;
> +const TAPE_CHANGER_MAX_LENGTH: usize = 32;
> +
>   pub const CHANGER_NAME_SCHEMA: Schema = StringSchema::new("Tape Changer Identifier.")
>       .format(&PROXMOX_SAFE_ID_FORMAT)
> -    .min_length(3)
> -    .max_length(32)
> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
> +    .schema();
> +
> +pub const CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT: Schema =

typo: ASIGNMENT vs ASSIGNMENT

> +    StringSchema::new("Tape Changer Identifier to be used for automatic tape drive assignment.")
> +    .format(&PROXMOX_SAFE_ID_FORMAT)
> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
>       .schema();
>   
>   pub const SCSI_CHANGER_PATH_SCHEMA: Schema =
> diff --git a/src/api2/tape/changer.rs b/src/api2/tape/changer.rs
> index 7ecf7bff..b73d6832 100644
> --- a/src/api2/tape/changer.rs
> +++ b/src/api2/tape/changer.rs
> @@ -1,6 +1,6 @@
>   use std::collections::HashMap;
>   
> -use anyhow::Error;
> +use anyhow::{bail, Error};
>   use serde_json::Value;
>   
>   use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
> @@ -8,7 +8,8 @@ use proxmox_schema::api;
>   
>   use pbs_api_types::{
>       Authid, ChangerListEntry, LtoTapeDrive, MtxEntryKind, MtxStatusEntry, ScsiTapeChanger,
> -    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ,
> +    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ, MEDIA_LABEL_SCHEMA, UPID_SCHEMA,
> +    DriveListEntry, DeviceActivity
>   };
>   use pbs_config::CachedUserInfo;
>   use pbs_tape::{
> @@ -199,7 +200,119 @@ pub fn list_changers(
>       Ok(list)
>   }
>   
> +#[api(
> +    input: {
> +        properties: {
> +            name: {
> +                schema: CHANGER_NAME_SCHEMA,
> +            },
> +            "label-text": {
> +                schema: MEDIA_LABEL_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["tape", "device", "{name}"], PRIV_TAPE_READ, false),
> +    },
> +)]
> +/// Load media with specified label
> +///
> +/// Issue a media load request to the associated changer device.
> +pub fn load_media(
> +    name: String,
> +    label_text: String,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +    let drive = choose_drive(&name, rpcenv);
> +    super::drive::load_media(drive?, label_text, rpcenv)
> +}> +
> +#[api(
> +    input: {
> +        properties: {
> +            name: {
> +                schema: CHANGER_NAME_SCHEMA,
> +            },
> +            "source-slot": {
> +                description: "Source slot number.",
> +                minimum: 1,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["tape", "device", "{name}"], PRIV_TAPE_READ, false),
> +    },
> +)]
> +/// Load media from the specified slot
> +///
> +/// Issue a media load request to the associated changer device.
> +pub async fn load_slot(
> +    name: String,
> +    source_slot: u64,
> +    rpcenv: &mut dyn RpcEnvironment,) -> Result<(), Error> {
> +    let drive = choose_drive(&name, rpcenv);
> +    super::drive::load_slot(drive?, source_slot).await
> +}
> +
> +fn choose_drive(
> +    changer: &str,
> +    rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
> +    // We have to automatically select a drive from the specified changer. For that purpose, we take all drives that are part of the changer
> +    // and search for the one that has not been used for the longest time (to ensure similar wear between the drives), or use one that has never been used.

Please keep the line length limit also for comments

> +    let drives = super::drive::list_drives(Option::from(changer.to_string()), true, Value::Null, rpcenv);
> +

I'd use `Some(x)` instead of `Option::from(x)` because it's shorter, and because it's
clearer what it does

> +    match drives {
> +        Ok(entries) => {

this pattern with a bail! in the Err case is imho much neater when written like this:

let entries = list_drives(..).map_err(|err| format_err!("cannot query drives: {err}"))?;

with that you eliminate a complete level of indentation for the remaining code

> +            let idle_drives: Vec<DriveListEntry> = entries.into_iter().filter(|entry| matches!(entry.activity, None | Some(DeviceActivity::NoActivity))).collect();
> +            let drives_info = crate::tape::drive_info::get_drives_info();
> +
> +            match drives_info {
> +                Ok(drives_info) => {
> +                    let mut index_oldest : Option<usize> = Option::default();
> +                    let mut oldest_time: Option<i64> = Option::default();
> +

I'd write

let mut foo: Option<T> = None;

here.

Yes, it's clear that None is the default for most rust developers, but
seeing the `None` explicitely here makes the code much more readable IMO


> +                    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
}

> +            }
> +        }
> +        Err(error) => bail!("cannot query drives: {}", error)
> +    }
> +}
> +
>   const SUBDIRS: SubdirMap = &[
> +    ("load-media", &Router::new().post(&API_METHOD_LOAD_MEDIA)),
> +    ("load-slot", &Router::new().post(&API_METHOD_LOAD_SLOT)),
>       ("status", &Router::new().get(&API_METHOD_GET_STATUS)),
>       ("transfer", &Router::new().post(&API_METHOD_TRANSFER)),
>   ];
> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
> index 7a72d49a..c6b2d532 100644
> --- a/src/bin/proxmox-backup-api.rs
> +++ b/src/bin/proxmox-backup-api.rs
> @@ -43,6 +43,8 @@ fn get_index() -> Pin<Box<dyn Future<Output = Response<Body>> + Send>> {
>   async fn run() -> Result<(), Error> {
>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>   
> +    let _ = proxmox_backup::tools::init_product_config();
> +

you introduce this method here, but always ignore the error.
what happens when the init fails? will the 'replace_config' panic later?
(that would be very bad?)

i think it would be better to split this out into it's own patch
with a good reasoning why the error is safe to be ignored (or not)

IMHO if the init fails, we cannot continue anyway, e.g.
in the main function of the backup-proxy we fail if  `backup_user()` fails

>       config::create_configdir()?;
>   
>       config::update_self_signed_cert(false)?;
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index ce1be1c0..eb7da0e4 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -181,6 +181,8 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
>   async fn run() -> Result<(), Error> {
>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>   
> +    let _ = proxmox_backup::tools::init_product_config();
> +
>       proxmox_backup::auth_helpers::setup_auth_context(false);
>       proxmox_backup::server::notifications::init()?;
>       metric_collection::init()?;
> diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
> index 8e8584b3..8de94af0 100644
> --- a/src/bin/proxmox-tape.rs
> +++ b/src/bin/proxmox-tape.rs
> @@ -1,6 +1,8 @@
>   use std::collections::HashMap;
>   
>   use anyhow::{bail, format_err, Error};
> +use pbs_api_types::CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT;
> +use pbs_config::drive::complete_changer_name;
>   use serde_json::{json, Value};
>   
>   use proxmox_human_byte::HumanByte;
> @@ -214,6 +216,10 @@ async fn eject_media(mut param: Value) -> Result<(), Error> {
>                   schema: DRIVE_NAME_SCHEMA,
>                   optional: true,
>               },
> +            changer: {
> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
> +                optional: true,
> +            },
>               "label-text": {
>                   schema: MEDIA_LABEL_SCHEMA,
>               },
> @@ -230,11 +236,25 @@ async fn load_media(mut param: Value) -> Result<(), Error> {
>   
>       let (config, _digest) = pbs_config::drive::config()?;
>   
> -    let drive = extract_drive_name(&mut param, &config)?;
> +    let drive = extract_drive_name(&mut param, &config);
> +    let changer = param["changer"].as_str();
> +
> +    let path = match changer {
> +        Some(changer) => {
> +            format!("api2/json/tape/changer/{}/load-media", changer)
> +        }
> +        None => {
> +            let drive = drive?;
> +            format!("api2/json/tape/drive/{}/load-media", drive)
> +        }
> +    };

here you could instead do the following:

let path = match (changer, drive) {
    (Some(changer), None) => // changer path
    (None, Some(drive)) => // drive path
    _ => // fail with error that exactly one of the options should be set
};

in your code, i can give both, but it will autodetect the drive even if
given explicitly when the changer is given, which might be confusing
for users

> +
> +    if let Some(param) = param.as_object_mut() {
> +        param.remove("changer");
> +    }
>   
>       let client = connect_to_localhost()?;
>   
> -    let path = format!("api2/json/tape/drive/{}/load-media", drive);
>       let result = client.post(&path, Some(param)).await?;
>   
>       view_task_result(&client, result, &output_format).await?;
> @@ -276,6 +296,10 @@ async fn export_media(mut param: Value) -> Result<(), Error> {
>                   schema: DRIVE_NAME_SCHEMA,
>                   optional: true,
>               },
> +            changer: {
> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
> +                optional: true,
> +            },
>               "source-slot": {
>                   description: "Source slot number.",
>                   type: u64,
> @@ -288,11 +312,25 @@ async fn export_media(mut param: Value) -> Result<(), Error> {
>   async fn load_media_from_slot(mut param: Value) -> Result<(), Error> {
>       let (config, _digest) = pbs_config::drive::config()?;
>   
> -    let drive = extract_drive_name(&mut param, &config)?;
> +    let drive = extract_drive_name(&mut param, &config);
> +    let changer = param["changer"].as_str();
> +
> +    let path = match changer {
> +        Some(changer) => {
> +            format!("api2/json/tape/changer/{}/load-slot", changer)
> +        }
> +        None => {
> +            let drive = drive?;
> +            format!("api2/json/tape/drive/{}/load-slot", drive)
> +        }
> +    };

the same here

> +
> +    if let Some(param) = param.as_object_mut() {
> +        param.remove("changer");
> +    }
>   
>       let client = connect_to_localhost()?;
>   
> -    let path = format!("api2/json/tape/drive/{}/load-slot", drive);
>       client.post(&path, Some(param)).await?;
>   
>       Ok(())
> @@ -1091,13 +1129,15 @@ fn main() {
>               CliCommand::new(&API_METHOD_LOAD_MEDIA)
>                   .arg_param(&["label-text"])
>                   .completion_cb("drive", complete_drive_name)
> +                .completion_cb("changer", complete_changer_name)
>                   .completion_cb("label-text", complete_media_label_text),
>           )
>           .insert(
>               "load-media-from-slot",
>               CliCommand::new(&API_METHOD_LOAD_MEDIA_FROM_SLOT)
>                   .arg_param(&["source-slot"])
> -                .completion_cb("drive", complete_drive_name),
> +                .completion_cb("drive", complete_drive_name)
> +                .completion_cb("changer", complete_changer_name),
>           )
>           .insert(
>               "unload",
> diff --git a/src/tape/changer/mod.rs b/src/tape/changer/mod.rs
> index 18ea0f46..8e3ff0f7 100644
> --- a/src/tape/changer/mod.rs
> +++ b/src/tape/changer/mod.rs
> @@ -273,6 +273,17 @@ pub trait MediaChange {
>       }
>   }
>   
> +/// Updates the drive's last usage time to now.
> +pub(super) fn update_drive_usage(drive: &str) -> Result<(), Error> {
> +    let _lock = crate::tape::drive_info::lock()?;
> +
> +    let mut drives_info = crate::tape::drive_info::get_drives_info()?;
> +
> +    let now = proxmox_time::epoch_i64();
> +    drives_info.drives.entry(drive.into()).or_default().last_usage = now;
> +    crate::tape::drive_info::save_config(&drives_info)
> +}
> +
>   const USE_MTX: bool = false;
>   
>   impl ScsiMediaChange for ScsiTapeChanger {
> @@ -423,7 +434,13 @@ impl MediaChange for MtxMediaChanger {
>       }
>   
>       fn load_media_from_slot(&mut self, slot: u64) -> Result<MtxStatus, Error> {
> -        self.config.load_slot(slot, self.drive_number())
> +        let status = self.config.load_slot(slot, self.drive_number())?;
> +
> +        if let Err(err) = update_drive_usage(self.drive_name()) {
> +            log::warn!("could not update drive usage: {err}");
> +        }
> +
> +        Ok(status)
>       }
>   
>       fn unload_media(&mut self, target_slot: Option<u64>) -> Result<MtxStatus, Error> {
> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
> index 866e4d32..213f17fe 100644
> --- a/src/tape/drive/virtual_tape.rs
> +++ b/src/tape/drive/virtual_tape.rs
> @@ -567,7 +567,13 @@ impl MediaChange for VirtualTapeHandle {
>           };
>           self.store_status(&status)?;
>   
> -        self.status()
> +        let status = self.status()?;
> +
> +        if let Err(err) = crate::tape::changer::update_drive_usage(self.drive_name()) {
> +            log::warn!("could not update drive usage: {err}");
> +        }
> +
> +        Ok(status)
>       }
>   
>       fn unload_media(&mut self, _target_slot: Option<u64>) -> Result<MtxStatus, Error> {
> 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)

> +
> +/// Get exclusive lock
> +pub fn lock() -> Result<ApiLockGuard, Error> {
> +    proxmox_product_config::open_api_lockfile(DRIVE_INFO_LOCKFILE, Option::None, true)
> +}
> +
> +/// Read and parse the drive info file
> +pub fn get_drives_info() -> Result<DrivesInfo, Error> {
> +    let content =
> +        proxmox_sys::fs::file_read_optional_string(DRIVE_INFO_FILENAME)?;
> +
> +    match content {
> +        Some(content) => {
> +            let result = serde_json::from_str::<DrivesInfo>(&content)?;
> +            Ok(result)
> +        },
> +        None => {
> +            Ok(DrivesInfo::default())
> +        }
> +    }
> +}
> +
> +/// Save the configuration file
> +pub fn save_config(data: &DrivesInfo) -> Result<(), Error> {
> +    let json = serde_json::to_string(data)?;
> +    proxmox_product_config::replace_config(DRIVE_INFO_FILENAME, json.as_bytes())
> +}
> diff --git a/src/tape/mod.rs b/src/tape/mod.rs
> index f276f948..8b87152d 100644
> --- a/src/tape/mod.rs
> +++ b/src/tape/mod.rs
> @@ -20,6 +20,7 @@ pub use inventory::*;
>   
>   pub mod changer;
>   pub mod drive;
> +pub mod drive_info;
>   pub mod encryption_keys;
>   
>   mod media_pool;
> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
> index 322894dd..11fb2821 100644
> --- a/src/tools/mod.rs
> +++ b/src/tools/mod.rs
> @@ -61,3 +61,10 @@ pub fn setup_safe_path_env() {
>           std::env::remove_var(name);
>       }
>   }
> +
> +pub fn init_product_config() -> Result<(), Error> {

like most of the functions in this file, a short comment would be nice,
even if it's just a reference to the `proxmox_product_config` crate

> +    let backup_user = pbs_config::backup_user()?;
> +    let root_user = nix::unistd::User::from_uid(nix::unistd::ROOT)?.expect("could not find root user");

this seems inconsistent.

for two things we bubble the error up, but a missing root user panics

I'd probably make that into an error that bubbles up too

> +    proxmox_product_config::init(backup_user, root_user);
> +    Ok(())
> +}


> diff --git a/www/tape/ChangerStatus.js b/www/tape/ChangerStatus.js
> index e18af90e..0a875779 100644
> --- a/www/tape/ChangerStatus.js
> +++ b/www/tape/ChangerStatus.js
> @@ -222,12 +222,17 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>   		    autoShow: true,
>   		    submitText: gettext('OK'),
>   		    title: gettext('Load Media into Drive'),
> -		    url: `/api2/extjs/tape/drive`,
> +		    url: `/api2/extjs/tape`,
>   		    method: 'POST',
>   		    submitUrl: function(url, values) {
> -			let drive = values.drive;
> -			delete values.drive;
> -			return `${url}/${encodeURIComponent(drive)}/${apiCall}`;
> +			    let drive = values.drive;
> +			    delete values.drive;
> +			
> +			    if (drive) {
> +				    return `${url}/drive/${encodeURIComponent(drive)}/${apiCall}`;
> +			    } else {
> +				    return `${url}/changer/${encodeURIComponent(changer)}/${apiCall}`;
> +			    }

nit: just so we don't have too many places where we create such urls,
I'd try to combine them into one:

let type = drive ? "drive" : "changer";
let item = drive ? drive : changer;

return `${url}/${type}/${encodeURIComponent(item)}/${apiCall}`;

or something like that

>   		    },
>   		    items: [
>   			label !== "" ? {
> @@ -248,6 +253,9 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>   			    fieldLabel: gettext('Drive'),
>   			    changer: changer,
>   			    name: 'drive',
> +			    emptyText: gettext('Choose Automatically'),
> +			    allowBlank: true,
> +			    autoSelect: false,
>   			},
>   		    ],
>   		    listeners: {



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes
  2025-01-28 12:25   ` Dominik Csapak
@ 2025-01-31 16:10     ` Laurențiu Leahu-Vlăducu
  2025-02-04  8:13       ` Dominik Csapak
  0 siblings, 1 reply; 6+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-31 16:10 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox Backup Server development discussion

Thanks for your review! I already made almost all the requested changes. 
Some further comments inline:

On 28.01.25 13:25, Dominik Csapak wrote:
> Thanks for tackling this!
> 
> a few high level comments:
> 
> * please write the relevant text as commit message (as mentioned off-list)
>    basically the text in the (here not needed) cover-letter would fit
>    as the commit message
> 
> * rustfmt: please use it, there are some lines that are very long.
>    comments should probably also not exceed the line length limit

Sorry, I thought rustfmt is running, but it was my editor's default 
fallback formatter due to the fact that it could not find the 
rust-analyzer (I forgot to install it). My bad. I fixed this already and 
will be available in the next version of the patch.

> 
> * it would be good to explain the reason for the code a bit more, e.g. 
> you introduce
>    the product-config crate here and do an initialization,
>    but neither by looking at the code, nor by reading the commit
>    message i know exactly why (i know it because we talked off-list)
>    a short sentence like:
> 
>    added the product-config crate so we're able to reuse the
>    'replace_config' instead of copying it.
> 
>    can already be enough
> 
> * the patch does not currently apply cleanly because we did
>    some code moves (e.g. pbs-api-types is now an external crate)
> 
> 
> Some other comments inline:
> 
> On 1/16/25 12:51, Laurențiu Leahu-Vlăducu wrote:
>> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
>> ---
>>   Cargo.toml                        |   2 +
>>   pbs-api-types/src/tape/changer.rs |  14 +++-
>>   src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
>>   src/bin/proxmox-backup-api.rs     |   2 +
>>   src/bin/proxmox-backup-proxy.rs   |   2 +
>>   src/bin/proxmox-tape.rs           |  50 +++++++++++--
>>   src/tape/changer/mod.rs           |  19 ++++-
>>   src/tape/drive/virtual_tape.rs    |   8 +-
>>   src/tape/drive_info.rs            |  51 +++++++++++++
>>   src/tape/mod.rs                   |   1 +
>>   src/tools/mod.rs                  |   7 ++
>>   www/tape/ChangerStatus.js         |  16 +++-
>>   12 files changed, 274 insertions(+), 15 deletions(-)
>>   create mode 100644 src/tape/drive_info.rs
>>
>> diff --git a/Cargo.toml b/Cargo.toml
>> index adeeb6ef..348d1cea 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -72,6 +72,7 @@ proxmox-ldap = "0.2.1"
>>   proxmox-metrics = "0.3.1"
>>   proxmox-notify = "0.5.1"
>>   proxmox-openid = "0.10.0"
>> +proxmox-product-config = "0.2.2"
>>   proxmox-rest-server = { version = "0.8.5", features = [ "templates" ] }
>>   # some use "cli", some use "cli" and "server", pbs-config uses nothing
>>   proxmox-router = { version = "3.0.0", default-features = false }
>> @@ -221,6 +222,7 @@ proxmox-ldap.workspace = true
>>   proxmox-metrics.workspace = true
>>   proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
>>   proxmox-openid.workspace = true
>> +proxmox-product-config.workspace = true
>>   proxmox-rest-server = { workspace = true, features = [ "rate- 
>> limited-stream" ] }
>>   proxmox-router = { workspace = true, features = [ "cli", "server"] }
>>   proxmox-schema = { workspace = true, features = [ "api-macro" ] }
>> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/ 
>> tape/changer.rs
>> index df3823cf..52ee08db 100644
>> --- a/pbs-api-types/src/tape/changer.rs
>> +++ b/pbs-api-types/src/tape/changer.rs
>> @@ -8,10 +8,20 @@ use proxmox_schema::{
>>   use crate::{OptionalDeviceIdentification, PROXMOX_SAFE_ID_FORMAT};
>> +const TAPE_CHANGER_MIN_LENGTH: usize = 3;
>> +const TAPE_CHANGER_MAX_LENGTH: usize = 32;
>> +
>>   pub const CHANGER_NAME_SCHEMA: Schema = StringSchema::new("Tape 
>> Changer Identifier.")
>>       .format(&PROXMOX_SAFE_ID_FORMAT)
>> -    .min_length(3)
>> -    .max_length(32)
>> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
>> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
>> +    .schema();
>> +
>> +pub const CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT: Schema =
> 
> typo: ASIGNMENT vs ASSIGNMENT
> 
>> +    StringSchema::new("Tape Changer Identifier to be used for 
>> automatic tape drive assignment.")
>> +    .format(&PROXMOX_SAFE_ID_FORMAT)
>> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
>> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
>>       .schema();
>>   pub const SCSI_CHANGER_PATH_SCHEMA: Schema =
>> diff --git a/src/api2/tape/changer.rs b/src/api2/tape/changer.rs
>> index 7ecf7bff..b73d6832 100644
>> --- a/src/api2/tape/changer.rs
>> +++ b/src/api2/tape/changer.rs
>> @@ -1,6 +1,6 @@
>>   use std::collections::HashMap;
>> -use anyhow::Error;
>> +use anyhow::{bail, Error};
>>   use serde_json::Value;
>>   use proxmox_router::{list_subdirs_api_method, Permission, Router, 
>> RpcEnvironment, SubdirMap};
>> @@ -8,7 +8,8 @@ use proxmox_schema::api;
>>   use pbs_api_types::{
>>       Authid, ChangerListEntry, LtoTapeDrive, MtxEntryKind, 
>> MtxStatusEntry, ScsiTapeChanger,
>> -    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ,
>> +    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ, 
>> MEDIA_LABEL_SCHEMA, UPID_SCHEMA,
>> +    DriveListEntry, DeviceActivity
>>   };
>>   use pbs_config::CachedUserInfo;
>>   use pbs_tape::{
>> @@ -199,7 +200,119 @@ pub fn list_changers(
>>       Ok(list)
>>   }
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: CHANGER_NAME_SCHEMA,
>> +            },
>> +            "label-text": {
>> +                schema: MEDIA_LABEL_SCHEMA,
>> +            },
>> +        },
>> +    },
>> +    returns: {
>> +        schema: UPID_SCHEMA,
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["tape", "device", 
>> "{name}"], PRIV_TAPE_READ, false),
>> +    },
>> +)]
>> +/// Load media with specified label
>> +///
>> +/// Issue a media load request to the associated changer device.
>> +pub fn load_media(
>> +    name: String,
>> +    label_text: String,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<Value, Error> {
>> +    let drive = choose_drive(&name, rpcenv);
>> +    super::drive::load_media(drive?, label_text, rpcenv)
>> +}> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: CHANGER_NAME_SCHEMA,
>> +            },
>> +            "source-slot": {
>> +                description: "Source slot number.",
>> +                minimum: 1,
>> +            },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["tape", "device", 
>> "{name}"], PRIV_TAPE_READ, false),
>> +    },
>> +)]
>> +/// Load media from the specified slot
>> +///
>> +/// Issue a media load request to the associated changer device.
>> +pub async fn load_slot(
>> +    name: String,
>> +    source_slot: u64,
>> +    rpcenv: &mut dyn RpcEnvironment,) -> Result<(), Error> {
>> +    let drive = choose_drive(&name, rpcenv);
>> +    super::drive::load_slot(drive?, source_slot).await
>> +}
>> +
>> +fn choose_drive(
>> +    changer: &str,
>> +    rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
>> +    // We have to automatically select a drive from the specified 
>> changer. For that purpose, we take all drives that are part of the 
>> changer
>> +    // and search for the one that has not been used for the longest 
>> time (to ensure similar wear between the drives), or use one that has 
>> never been used.
> 
> Please keep the line length limit also for comments
> 
>> +    let drives = 
>> super::drive::list_drives(Option::from(changer.to_string()), true, 
>> Value::Null, rpcenv);
>> +
> 
> I'd use `Some(x)` instead of `Option::from(x)` because it's shorter, and 
> because it's
> clearer what it does
> 
>> +    match drives {
>> +        Ok(entries) => {
> 
> this pattern with a bail! in the Err case is imho much neater when 
> written like this:
> 
> let entries = list_drives(..).map_err(|err| format_err!("cannot query 
> drives: {err}"))?;
> 
> with that you eliminate a complete level of indentation for the 
> remaining code
> 
>> +            let idle_drives: Vec<DriveListEntry> = 
>> entries.into_iter().filter(|entry| matches!(entry.activity, None | 
>> Some(DeviceActivity::NoActivity))).collect();
>> +            let drives_info = 
>> crate::tape::drive_info::get_drives_info();
>> +
>> +            match drives_info {
>> +                Ok(drives_info) => {
>> +                    let mut index_oldest : Option<usize> = 
>> Option::default();
>> +                    let mut oldest_time: Option<i64> = 
>> Option::default();
>> +
> 
> I'd write
> 
> let mut foo: Option<T> = None;
> 
> here.
> 
> Yes, it's clear that None is the default for most rust developers, but
> seeing the `None` explicitely here makes the code much more readable IMO
> 
> 
>> +                    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.

>> +            }
>> +        }
>> +        Err(error) => bail!("cannot query drives: {}", error)
>> +    }
>> +}
>> +
>>   const SUBDIRS: SubdirMap = &[
>> +    ("load-media", &Router::new().post(&API_METHOD_LOAD_MEDIA)),
>> +    ("load-slot", &Router::new().post(&API_METHOD_LOAD_SLOT)),
>>       ("status", &Router::new().get(&API_METHOD_GET_STATUS)),
>>       ("transfer", &Router::new().post(&API_METHOD_TRANSFER)),
>>   ];
>> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup- 
>> api.rs
>> index 7a72d49a..c6b2d532 100644
>> --- a/src/bin/proxmox-backup-api.rs
>> +++ b/src/bin/proxmox-backup-api.rs
>> @@ -43,6 +43,8 @@ fn get_index() -> Pin<Box<dyn Future<Output = 
>> Response<Body>> + Send>> {
>>   async fn run() -> Result<(), Error> {
>>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>> +    let _ = proxmox_backup::tools::init_product_config();
>> +
> 
> you introduce this method here, but always ignore the error.
> what happens when the init fails? will the 'replace_config' panic later?
> (that would be very bad?)
> 
> i think it would be better to split this out into it's own patch
> with a good reasoning why the error is safe to be ignored (or not)
> 
> IMHO if the init fails, we cannot continue anyway, e.g.
> in the main function of the backup-proxy we fail if  `backup_user()` fails
> 
>>       config::create_configdir()?;
>>       config::update_self_signed_cert(false)?;
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup- 
>> proxy.rs
>> index ce1be1c0..eb7da0e4 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -181,6 +181,8 @@ async fn get_index_future(env: RestEnvironment, 
>> parts: Parts) -> Response<Body>
>>   async fn run() -> Result<(), Error> {
>>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>> +    let _ = proxmox_backup::tools::init_product_config();
>> +
>>       proxmox_backup::auth_helpers::setup_auth_context(false);
>>       proxmox_backup::server::notifications::init()?;
>>       metric_collection::init()?;
>> diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
>> index 8e8584b3..8de94af0 100644
>> --- a/src/bin/proxmox-tape.rs
>> +++ b/src/bin/proxmox-tape.rs
>> @@ -1,6 +1,8 @@
>>   use std::collections::HashMap;
>>   use anyhow::{bail, format_err, Error};
>> +use pbs_api_types::CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT;
>> +use pbs_config::drive::complete_changer_name;
>>   use serde_json::{json, Value};
>>   use proxmox_human_byte::HumanByte;
>> @@ -214,6 +216,10 @@ async fn eject_media(mut param: Value) -> 
>> Result<(), Error> {
>>                   schema: DRIVE_NAME_SCHEMA,
>>                   optional: true,
>>               },
>> +            changer: {
>> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
>> +                optional: true,
>> +            },
>>               "label-text": {
>>                   schema: MEDIA_LABEL_SCHEMA,
>>               },
>> @@ -230,11 +236,25 @@ async fn load_media(mut param: Value) -> 
>> Result<(), Error> {
>>       let (config, _digest) = pbs_config::drive::config()?;
>> -    let drive = extract_drive_name(&mut param, &config)?;
>> +    let drive = extract_drive_name(&mut param, &config);
>> +    let changer = param["changer"].as_str();
>> +
>> +    let path = match changer {
>> +        Some(changer) => {
>> +            format!("api2/json/tape/changer/{}/load-media", changer)
>> +        }
>> +        None => {
>> +            let drive = drive?;
>> +            format!("api2/json/tape/drive/{}/load-media", drive)
>> +        }
>> +    };
> 
> here you could instead do the following:
> 
> let path = match (changer, drive) {
>     (Some(changer), None) => // changer path
>     (None, Some(drive)) => // drive path
>     _ => // fail with error that exactly one of the options should be set
> };
> 
> in your code, i can give both, but it will autodetect the drive even if
> given explicitly when the changer is given, which might be confusing
> for users
> 
>> +
>> +    if let Some(param) = param.as_object_mut() {
>> +        param.remove("changer");
>> +    }
>>       let client = connect_to_localhost()?;
>> -    let path = format!("api2/json/tape/drive/{}/load-media", drive);
>>       let result = client.post(&path, Some(param)).await?;
>>       view_task_result(&client, result, &output_format).await?;
>> @@ -276,6 +296,10 @@ async fn export_media(mut param: Value) -> 
>> Result<(), Error> {
>>                   schema: DRIVE_NAME_SCHEMA,
>>                   optional: true,
>>               },
>> +            changer: {
>> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
>> +                optional: true,
>> +            },
>>               "source-slot": {
>>                   description: "Source slot number.",
>>                   type: u64,
>> @@ -288,11 +312,25 @@ async fn export_media(mut param: Value) -> 
>> Result<(), Error> {
>>   async fn load_media_from_slot(mut param: Value) -> Result<(), Error> {
>>       let (config, _digest) = pbs_config::drive::config()?;
>> -    let drive = extract_drive_name(&mut param, &config)?;
>> +    let drive = extract_drive_name(&mut param, &config);
>> +    let changer = param["changer"].as_str();
>> +
>> +    let path = match changer {
>> +        Some(changer) => {
>> +            format!("api2/json/tape/changer/{}/load-slot", changer)
>> +        }
>> +        None => {
>> +            let drive = drive?;
>> +            format!("api2/json/tape/drive/{}/load-slot", drive)
>> +        }
>> +    };
> 
> the same here
> 
>> +
>> +    if let Some(param) = param.as_object_mut() {
>> +        param.remove("changer");
>> +    }
>>       let client = connect_to_localhost()?;
>> -    let path = format!("api2/json/tape/drive/{}/load-slot", drive);
>>       client.post(&path, Some(param)).await?;
>>       Ok(())
>> @@ -1091,13 +1129,15 @@ fn main() {
>>               CliCommand::new(&API_METHOD_LOAD_MEDIA)
>>                   .arg_param(&["label-text"])
>>                   .completion_cb("drive", complete_drive_name)
>> +                .completion_cb("changer", complete_changer_name)
>>                   .completion_cb("label-text", 
>> complete_media_label_text),
>>           )
>>           .insert(
>>               "load-media-from-slot",
>>               CliCommand::new(&API_METHOD_LOAD_MEDIA_FROM_SLOT)
>>                   .arg_param(&["source-slot"])
>> -                .completion_cb("drive", complete_drive_name),
>> +                .completion_cb("drive", complete_drive_name)
>> +                .completion_cb("changer", complete_changer_name),
>>           )
>>           .insert(
>>               "unload",
>> diff --git a/src/tape/changer/mod.rs b/src/tape/changer/mod.rs
>> index 18ea0f46..8e3ff0f7 100644
>> --- a/src/tape/changer/mod.rs
>> +++ b/src/tape/changer/mod.rs
>> @@ -273,6 +273,17 @@ pub trait MediaChange {
>>       }
>>   }
>> +/// Updates the drive's last usage time to now.
>> +pub(super) fn update_drive_usage(drive: &str) -> Result<(), Error> {
>> +    let _lock = crate::tape::drive_info::lock()?;
>> +
>> +    let mut drives_info = crate::tape::drive_info::get_drives_info()?;
>> +
>> +    let now = proxmox_time::epoch_i64();
>> +    drives_info.drives.entry(drive.into()).or_default().last_usage = 
>> now;
>> +    crate::tape::drive_info::save_config(&drives_info)
>> +}
>> +
>>   const USE_MTX: bool = false;
>>   impl ScsiMediaChange for ScsiTapeChanger {
>> @@ -423,7 +434,13 @@ impl MediaChange for MtxMediaChanger {
>>       }
>>       fn load_media_from_slot(&mut self, slot: u64) -> 
>> Result<MtxStatus, Error> {
>> -        self.config.load_slot(slot, self.drive_number())
>> +        let status = self.config.load_slot(slot, self.drive_number())?;
>> +
>> +        if let Err(err) = update_drive_usage(self.drive_name()) {
>> +            log::warn!("could not update drive usage: {err}");
>> +        }
>> +
>> +        Ok(status)
>>       }
>>       fn unload_media(&mut self, target_slot: Option<u64>) -> 
>> Result<MtxStatus, Error> {
>> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/ 
>> virtual_tape.rs
>> index 866e4d32..213f17fe 100644
>> --- a/src/tape/drive/virtual_tape.rs
>> +++ b/src/tape/drive/virtual_tape.rs
>> @@ -567,7 +567,13 @@ impl MediaChange for VirtualTapeHandle {
>>           };
>>           self.store_status(&status)?;
>> -        self.status()
>> +        let status = self.status()?;
>> +
>> +        if let Err(err) = 
>> crate::tape::changer::update_drive_usage(self.drive_name()) {
>> +            log::warn!("could not update drive usage: {err}");
>> +        }
>> +
>> +        Ok(status)
>>       }
>>       fn unload_media(&mut self, _target_slot: Option<u64>) -> 
>> Result<MtxStatus, Error> {
>> 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 :)

>> +
>> +/// Get exclusive lock
>> +pub fn lock() -> Result<ApiLockGuard, Error> {
>> +    proxmox_product_config::open_api_lockfile(DRIVE_INFO_LOCKFILE, 
>> Option::None, true)
>> +}
>> +
>> +/// Read and parse the drive info file
>> +pub fn get_drives_info() -> Result<DrivesInfo, Error> {
>> +    let content =
>> +        
>> proxmox_sys::fs::file_read_optional_string(DRIVE_INFO_FILENAME)?;
>> +
>> +    match content {
>> +        Some(content) => {
>> +            let result = serde_json::from_str::<DrivesInfo>(&content)?;
>> +            Ok(result)
>> +        },
>> +        None => {
>> +            Ok(DrivesInfo::default())
>> +        }
>> +    }
>> +}
>> +
>> +/// Save the configuration file
>> +pub fn save_config(data: &DrivesInfo) -> Result<(), Error> {
>> +    let json = serde_json::to_string(data)?;
>> +    proxmox_product_config::replace_config(DRIVE_INFO_FILENAME, 
>> json.as_bytes())
>> +}
>> diff --git a/src/tape/mod.rs b/src/tape/mod.rs
>> index f276f948..8b87152d 100644
>> --- a/src/tape/mod.rs
>> +++ b/src/tape/mod.rs
>> @@ -20,6 +20,7 @@ pub use inventory::*;
>>   pub mod changer;
>>   pub mod drive;
>> +pub mod drive_info;
>>   pub mod encryption_keys;
>>   mod media_pool;
>> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
>> index 322894dd..11fb2821 100644
>> --- a/src/tools/mod.rs
>> +++ b/src/tools/mod.rs
>> @@ -61,3 +61,10 @@ pub fn setup_safe_path_env() {
>>           std::env::remove_var(name);
>>       }
>>   }
>> +
>> +pub fn init_product_config() -> Result<(), Error> {
> 
> like most of the functions in this file, a short comment would be nice,
> even if it's just a reference to the `proxmox_product_config` crate
> 
>> +    let backup_user = pbs_config::backup_user()?;
>> +    let root_user = 
>> nix::unistd::User::from_uid(nix::unistd::ROOT)?.expect("could not find 
>> root user");
> 
> this seems inconsistent.
> 
> for two things we bubble the error up, but a missing root user panics
> 
> I'd probably make that into an error that bubbles up too
> 
>> +    proxmox_product_config::init(backup_user, root_user);
>> +    Ok(())
>> +}
> 
> 
>> diff --git a/www/tape/ChangerStatus.js b/www/tape/ChangerStatus.js
>> index e18af90e..0a875779 100644
>> --- a/www/tape/ChangerStatus.js
>> +++ b/www/tape/ChangerStatus.js
>> @@ -222,12 +222,17 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>>               autoShow: true,
>>               submitText: gettext('OK'),
>>               title: gettext('Load Media into Drive'),
>> -            url: `/api2/extjs/tape/drive`,
>> +            url: `/api2/extjs/tape`,
>>               method: 'POST',
>>               submitUrl: function(url, values) {
>> -            let drive = values.drive;
>> -            delete values.drive;
>> -            return `${url}/${encodeURIComponent(drive)}/${apiCall}`;
>> +                let drive = values.drive;
>> +                delete values.drive;
>> +
>> +                if (drive) {
>> +                    return `${url}/drive/ 
>> ${encodeURIComponent(drive)}/${apiCall}`;
>> +                } else {
>> +                    return `${url}/changer/ 
>> ${encodeURIComponent(changer)}/${apiCall}`;
>> +                }
> 
> nit: just so we don't have too many places where we create such urls,
> I'd try to combine them into one:
> 
> let type = drive ? "drive" : "changer";
> let item = drive ? drive : changer;
> 
> return `${url}/${type}/${encodeURIComponent(item)}/${apiCall}`;
> 
> or something like that
> 
>>               },
>>               items: [
>>               label !== "" ? {
>> @@ -248,6 +253,9 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>>                   fieldLabel: gettext('Drive'),
>>                   changer: changer,
>>                   name: 'drive',
>> +                emptyText: gettext('Choose Automatically'),
>> +                allowBlank: true,
>> +                autoSelect: false,
>>               },
>>               ],
>>               listeners: {
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes
  2025-01-31 16:10     ` Laurențiu Leahu-Vlăducu
@ 2025-02-04  8:13       ` Dominik Csapak
  0 siblings, 0 replies; 6+ messages in thread
From: Dominik Csapak @ 2025-02-04  8:13 UTC (permalink / raw)
  To: Laurențiu Leahu-Vlăducu,
	Proxmox Backup Server development discussion

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.





_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pbs-devel] [PATCH proxmox-backup] Automatically select a drive (if part of a changer) when loading tapes
@ 2025-01-16 11:48 Laurențiu Leahu-Vlăducu
  0 siblings, 0 replies; 6+ messages in thread
From: Laurențiu Leahu-Vlăducu @ 2025-01-16 11:48 UTC (permalink / raw)
  To: pbs-devel; +Cc: Laurențiu Leahu-Vlăducu

This patch adds the possibility to load tapes into drives automatically
by specifying a changer. This is useful for larger tape libraries.
Choosing a drive is done by using the drive that has not been used the
longest.

At the moment, this patch implements the functionality for automatic
loading both over the API and using the proxmox-backup CLI tool, as well
as in the web UI when selecting a changer. A second patch will add the
same functionality when configuring backup jobs.

Partially fixes #3351


Laurențiu Leahu-Vlăducu (1):
  Automatically select a drive (if part of a changer) when loading tapes

 Cargo.toml                        |   2 +
 pbs-api-types/src/tape/changer.rs |  14 +++-
 src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
 src/bin/proxmox-backup-api.rs     |   2 +
 src/bin/proxmox-backup-proxy.rs   |   2 +
 src/bin/proxmox-tape.rs           |  50 +++++++++++--
 src/tape/changer/mod.rs           |  19 ++++-
 src/tape/drive/virtual_tape.rs    |   8 +-
 src/tape/drive_info.rs            |  51 +++++++++++++
 src/tape/mod.rs                   |   1 +
 src/tools/mod.rs                  |   7 ++
 www/tape/ChangerStatus.js         |  16 +++-
 12 files changed, 274 insertions(+), 15 deletions(-)
 create mode 100644 src/tape/drive_info.rs

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-02-04  8:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-16 11:51 [pbs-devel] [PATCH proxmox-backup] Automatically select a drive (if part of a changer) when loading tapes Laurențiu Leahu-Vlăducu
2025-01-16 11:51 ` [pbs-devel] [PATCH proxmox-backup 1/1] " Laurențiu Leahu-Vlăducu
2025-01-28 12:25   ` Dominik Csapak
2025-01-31 16:10     ` Laurențiu Leahu-Vlăducu
2025-02-04  8:13       ` Dominik Csapak
  -- strict thread matches above, loose matches on Subject: below --
2025-01-16 11:48 [pbs-devel] [PATCH proxmox-backup] " Laurențiu Leahu-Vlăducu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal