* [pbs-devel] [PATCH proxmox-backup v13 01/26] tools: add disks utility functions
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-17 19:34 ` [pbs-devel] applied: " Thomas Lamprecht
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 02/26] config: factor out method to get the absolute datastore path Hannes Laimer
` (25 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
... for mounting and unmounting
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* use &Path everywhere, instead of &str
src/tools/disks/mod.rs | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 8c479e94..10c4eed0 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1338,3 +1338,33 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
bail!("get_fs_uuid failed - missing UUID");
}
+
+/// Mount a disk by its UUID and the mount point.
+pub fn mount_by_uuid(uuid: &str, mount_point: &Path) -> Result<(), Error> {
+ let mut command = std::process::Command::new("mount");
+ command.arg(&format!("UUID={uuid}"));
+ command.arg(mount_point);
+
+ proxmox_sys::command::run_command(command, None)?;
+ Ok(())
+}
+
+/// Create bind mount.
+pub fn bind_mount(path: &Path, target: &Path) -> Result<(), Error> {
+ let mut command = std::process::Command::new("mount");
+ command.arg("--bind");
+ command.arg(path);
+ command.arg(target);
+
+ proxmox_sys::command::run_command(command, None)?;
+ Ok(())
+}
+
+/// Unmount a disk by its mount point.
+pub fn unmount_by_mountpoint(path: &Path) -> Result<(), Error> {
+ let mut command = std::process::Command::new("umount");
+ command.arg(path);
+
+ proxmox_sys::command::run_command(command, None)?;
+ Ok(())
+}
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 02/26] config: factor out method to get the absolute datastore path
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 01/26] tools: add disks utility functions Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-17 19:34 ` [pbs-devel] applied: " Thomas Lamprecht
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig Hannes Laimer
` (24 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
From: Dietmar Maurer <dietmar@proxmox.com>
removable datastores will have a PBS-managed mountpoint as path, direct
access to the field needs to be replaced with a helper that can account
for this.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* just commit msg
pbs-api-types/src/datastore.rs | 5 +++++
pbs-datastore/src/datastore.rs | 11 +++++++----
src/api2/node/disks/directory.rs | 4 ++--
src/server/metric_collection/mod.rs | 8 ++++++--
4 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 31767417..a5704c93 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -357,6 +357,11 @@ impl DataStoreConfig {
}
}
+ /// Returns the absolute path to the datastore content.
+ pub fn absolute_path(&self) -> String {
+ self.path.clone()
+ }
+
pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
self.maintenance_mode.as_ref().and_then(|str| {
MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index d0f3c53a..fb37bd5a 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -178,7 +178,7 @@ impl DataStore {
)?;
Arc::new(ChunkStore::open(
name,
- &config.path,
+ config.absolute_path(),
tuning.sync_level.unwrap_or_default(),
)?)
};
@@ -262,8 +262,11 @@ impl DataStore {
DatastoreTuning::API_SCHEMA
.parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
)?;
- let chunk_store =
- ChunkStore::open(&name, &config.path, tuning.sync_level.unwrap_or_default())?;
+ let chunk_store = ChunkStore::open(
+ &name,
+ config.absolute_path(),
+ tuning.sync_level.unwrap_or_default(),
+ )?;
let inner = Arc::new(Self::with_store_and_config(
Arc::new(chunk_store),
config,
@@ -1387,7 +1390,7 @@ impl DataStore {
bail!("datastore is currently in use");
}
- let base = PathBuf::from(&datastore_config.path);
+ let base = PathBuf::from(datastore_config.absolute_path());
let mut ok = true;
if destroy_data {
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 06ad5ba1..7f540220 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -249,12 +249,12 @@ pub fn delete_datastore_disk(name: String) -> Result<(), Error> {
let (config, _) = pbs_config::datastore::config()?;
let datastores: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
let conflicting_datastore: Option<DataStoreConfig> =
- datastores.into_iter().find(|ds| ds.path == path);
+ datastores.into_iter().find(|ds| ds.absolute_path() == path);
if let Some(conflicting_datastore) = conflicting_datastore {
bail!(
"Can't remove '{}' since it's required by datastore '{}'",
- conflicting_datastore.path,
+ conflicting_datastore.absolute_path(),
conflicting_datastore.name
);
}
diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
index 3cbd7425..b95dba20 100644
--- a/src/server/metric_collection/mod.rs
+++ b/src/server/metric_collection/mod.rs
@@ -175,8 +175,12 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DiskStat>) {
{
continue;
}
- let path = Path::new(&config.path);
- datastores.push(gather_disk_stats(disk_manager.clone(), path, &config.name));
+
+ datastores.push(gather_disk_stats(
+ disk_manager.clone(),
+ Path::new(&config.absolute_path()),
+ &config.name,
+ ));
}
}
Err(err) => {
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 01/26] tools: add disks utility functions Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 02/26] config: factor out method to get the absolute datastore path Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-17 19:27 ` Thomas Lamprecht
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 04/26] maintenance: add 'Unmount' maintenance type Hannes Laimer
` (23 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* clearify/improve description of `DATASTORE_DIR_NAME_SCHAME`
pbs-api-types/src/datastore.rs | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index a5704c93..f6c255d3 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -42,7 +42,7 @@ const_regex! {
pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);
-pub const DIR_NAME_SCHEMA: Schema = StringSchema::new("Directory name")
+pub const DATASTORE_DIR_NAME_SCHEMA: Schema = StringSchema::new("Either the absolute path to the datastore directory, or a relative on-device path for removable datastores.")
.min_length(1)
.max_length(4096)
.schema();
@@ -160,6 +160,9 @@ pub const PRUNE_SCHEMA_KEEP_YEARLY: Schema =
.minimum(1)
.schema();
+/// Base directory where datastores are mounted
+pub const DATASTORE_MOUNT_DIR: &str = "/mnt/datastore";
+
#[api]
#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
@@ -234,7 +237,7 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
schema: DATASTORE_SCHEMA,
},
path: {
- schema: DIR_NAME_SCHEMA,
+ schema: DATASTORE_DIR_NAME_SCHEMA,
},
"notify-user": {
optional: true,
@@ -273,6 +276,12 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
format: &ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA),
type: String,
},
+ "backing-device": {
+ description: "The UUID of the filesystem partition for removable datastores.",
+ optional: true,
+ format: &proxmox_schema::api_types::UUID_FORMAT,
+ type: String,
+ }
}
)]
#[derive(Serialize, Deserialize, Updater, Clone, PartialEq)]
@@ -320,6 +329,11 @@ pub struct DataStoreConfig {
/// Maintenance mode, type is either 'offline' or 'read-only', message should be enclosed in "
#[serde(skip_serializing_if = "Option::is_none")]
pub maintenance_mode: Option<String>,
+
+ /// The UUID of the device(for removable datastores)
+ #[updater(skip)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub backing_device: Option<String>,
}
#[api]
@@ -354,12 +368,23 @@ impl DataStoreConfig {
notification_mode: None,
tuning: None,
maintenance_mode: None,
+ backing_device: None,
}
}
/// Returns the absolute path to the datastore content.
pub fn absolute_path(&self) -> String {
- self.path.clone()
+ if self.backing_device.is_some() {
+ format!("{DATASTORE_MOUNT_DIR}/{}", self.name)
+ } else {
+ self.path.clone()
+ }
+ }
+
+ pub fn get_mount_point(&self) -> Option<String> {
+ self.backing_device
+ .is_some()
+ .then(|| format!("{DATASTORE_MOUNT_DIR}/{}", self.name))
}
pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig Hannes Laimer
@ 2024-11-17 19:27 ` Thomas Lamprecht
2024-11-18 8:36 ` Hannes Laimer
2024-11-27 15:50 ` Thomas Lamprecht
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Lamprecht @ 2024-11-17 19:27 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Hannes Laimer
Am 13.11.24 um 16:00 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * clearify/improve description of `DATASTORE_DIR_NAME_SCHAME`
>
> pbs-api-types/src/datastore.rs | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index a5704c93..f6c255d3 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -42,7 +42,7 @@ const_regex! {
>
> pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);
>
> -pub const DIR_NAME_SCHEMA: Schema = StringSchema::new("Directory name")
> +pub const DATASTORE_DIR_NAME_SCHEMA: Schema = StringSchema::new("Either the absolute path to the datastore directory, or a relative on-device path for removable datastores.")
> .min_length(1)
> .max_length(4096)
> .schema();
> @@ -160,6 +160,9 @@ pub const PRUNE_SCHEMA_KEEP_YEARLY: Schema =
> .minimum(1)
> .schema();
>
> +/// Base directory where datastores are mounted
> +pub const DATASTORE_MOUNT_DIR: &str = "/mnt/datastore";
> +
> #[api]
> #[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
> #[serde(rename_all = "lowercase")]
> @@ -234,7 +237,7 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
> schema: DATASTORE_SCHEMA,
> },
> path: {
> - schema: DIR_NAME_SCHEMA,
> + schema: DATASTORE_DIR_NAME_SCHEMA,
> },
> "notify-user": {
> optional: true,
> @@ -273,6 +276,12 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
> format: &ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA),
> type: String,
> },
> + "backing-device": {
> + description: "The UUID of the filesystem partition for removable datastores.",
> + optional: true,
> + format: &proxmox_schema::api_types::UUID_FORMAT,
FWIW, I get an error about this regex not matching for a USB pen drive I'm testing.
The POST data is:
{
"name": "samsung-stick-foo",
"path": "foo",
"backing-device": "64A5-F009",
"gc-schedule": "daily",
"prune-schedule": "daily",
"comment": "",
"notification-mode": "notification-system"
}
The data of the usb disk I selected
{
"3": {
"devpath": "/dev/sdd",
"disk-type": "hdd",
"gpt": false,
"model": "Flash_Drive_FIT",
"name": "sdd",
"partitions": [
{
"devpath": "/dev/sdd1",
"filesystem": "exfat",
"gpt": false,
"mounted": false,
"name": "sdd1",
"size": 128320719872,
"used": "filesystem",
"uuid": "64A5-F009"
}
],
"rpm": null,
"serial": "0392523110004665",
"size": 128320801792,
"status": "unknown",
"used": "partitions",
"vendor": "Samsung",
"wearout": null,
"wwn": null
}
}
note: this pen drive is brand new, got just unwrapped and passed through to my dev
VM, and as such it's still coming with the formatting from factoring.
Now, I first did not even expect that it shows up in the selector, but it did, so I'm
wondering if it either should not be available or if it should work to use this disk
too.
No worries, I do not want an immediate fix or the like, just would like to know what's
the expected outcome here is – as I think quite some other users might also plug in their
freshly unwrapped and proudly exfat/vfat formatted pen drives to see how this goes.
That they have to do something might be fine, but a regex not matching error won't
shove them in the right direction I think.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig
2024-11-17 19:27 ` Thomas Lamprecht
@ 2024-11-18 8:36 ` Hannes Laimer
2024-11-27 15:50 ` Thomas Lamprecht
1 sibling, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-18 8:36 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On Sun Nov 17, 2024 at 8:27 PM CET, Thomas Lamprecht wrote:
> Am 13.11.24 um 16:00 schrieb Hannes Laimer:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> changes since v12:
>> * clearify/improve description of `DATASTORE_DIR_NAME_SCHAME`
>>
>> pbs-api-types/src/datastore.rs | 31 ++++++++++++++++++++++++++++---
>> 1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index a5704c93..f6c255d3 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -42,7 +42,7 @@ const_regex! {
>>
>> pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);
>>
>> -pub const DIR_NAME_SCHEMA: Schema = StringSchema::new("Directory name")
>> +pub const DATASTORE_DIR_NAME_SCHEMA: Schema = StringSchema::new("Either the absolute path to the datastore directory, or a relative on-device path for removable datastores.")
>> .min_length(1)
>> .max_length(4096)
>> .schema();
>> @@ -160,6 +160,9 @@ pub const PRUNE_SCHEMA_KEEP_YEARLY: Schema =
>> .minimum(1)
>> .schema();
>>
>> +/// Base directory where datastores are mounted
>> +pub const DATASTORE_MOUNT_DIR: &str = "/mnt/datastore";
>> +
>> #[api]
>> #[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
>> #[serde(rename_all = "lowercase")]
>> @@ -234,7 +237,7 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
>> schema: DATASTORE_SCHEMA,
>> },
>> path: {
>> - schema: DIR_NAME_SCHEMA,
>> + schema: DATASTORE_DIR_NAME_SCHEMA,
>> },
>> "notify-user": {
>> optional: true,
>> @@ -273,6 +276,12 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
>> format: &ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA),
>> type: String,
>> },
>> + "backing-device": {
>> + description: "The UUID of the filesystem partition for removable datastores.",
>> + optional: true,
>> + format: &proxmox_schema::api_types::UUID_FORMAT,
>
> FWIW, I get an error about this regex not matching for a USB pen drive I'm testing.
>
> The POST data is:
>
> {
> "name": "samsung-stick-foo",
> "path": "foo",
> "backing-device": "64A5-F009",
> "gc-schedule": "daily",
> "prune-schedule": "daily",
> "comment": "",
> "notification-mode": "notification-system"
> }
>
>
> The data of the usb disk I selected
>
> {
> "3": {
> "devpath": "/dev/sdd",
> "disk-type": "hdd",
> "gpt": false,
> "model": "Flash_Drive_FIT",
> "name": "sdd",
> "partitions": [
> {
> "devpath": "/dev/sdd1",
> "filesystem": "exfat",
> "gpt": false,
> "mounted": false,
> "name": "sdd1",
> "size": 128320719872,
> "used": "filesystem",
> "uuid": "64A5-F009"
> }
> ],
> "rpm": null,
> "serial": "0392523110004665",
> "size": 128320801792,
> "status": "unknown",
> "used": "partitions",
> "vendor": "Samsung",
> "wearout": null,
> "wwn": null
> }
> }
>
> note: this pen drive is brand new, got just unwrapped and passed through to my dev
> VM, and as such it's still coming with the formatting from factoring.
>
> Now, I first did not even expect that it shows up in the selector, but it did, so I'm
> wondering if it either should not be available or if it should work to use this disk
> too.
>
> No worries, I do not want an immediate fix or the like, just would like to know what's
> the expected outcome here is – as I think quite some other users might also plug in their
> freshly unwrapped and proudly exfat/vfat formatted pen drives to see how this goes.
> That they have to do something might be fine, but a regex not matching error won't
> shove them in the right direction I think.
Good point, we could either
- not reutrn UUIDs that are not really UUIDs[1]
- filter them out in the UI
In both cases the supported filesystem should probably be mentioned to the error msg.
Generally I think it'd be better to not return a UUID that is not really
a UUID. So since, for example, `exfat` only has a 'pseudo-UUID' it would
not be included in the returned data.
[1] https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-format
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig
2024-11-17 19:27 ` Thomas Lamprecht
2024-11-18 8:36 ` Hannes Laimer
@ 2024-11-27 15:50 ` Thomas Lamprecht
1 sibling, 0 replies; 45+ messages in thread
From: Thomas Lamprecht @ 2024-11-27 15:50 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Hannes Laimer
Am 17.11.24 um 20:27 schrieb Thomas Lamprecht:
> note: this pen drive is brand new, got just unwrapped and passed through to my dev
> VM, and as such it's still coming with the formatting from factoring.
>
> Now, I first did not even expect that it shows up in the selector, but it did, so I'm
> wondering if it either should not be available or if it should work to use this disk
> too.
For the record, the main blocker here are:
1. that exfat/vfat does not support arbitrary UID/GID for files, so a datastore creation
fails when PBS tries to assign the directories and files ownership to the backup user
and group. While not really nice, that can be worked around by using the
`-ouid=34,gid=34` mount options.
2. more importantly, a too low max dentry per directory limit, causing ENOSPACE errors
on chunkstore creation. While this could be workarounded by creating deeper chunkstore
levels with fewer directories per level, e.g. a three-letter prefix per level, which is
coming out a 3 * 4 bits = 12 -> 4096 directories per level, and that for three or four
levels. But that's rather a bigger change, and having different layouts per filesystem
type sounds like it could cause quite a few issues, not what one wants for a stable
backup solution.
3. Maybe more? we did not get around to even create a datastore, so actual usage and
support for locks and all that might be other blockers.
So it's simply not feasible to support vfat, or other limited FS.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 04/26] maintenance: add 'Unmount' maintenance type
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (2 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 03/26] pbs-api-types: add backing-device to DataStoreConfig Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 05/26] disks: add UUID to partition info Hannes Laimer
` (22 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
From: Dietmar Maurer <dietmar@proxmox.com>
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
I don't remember exactly, but ITRC this is only part of Dietmar's
original patch. Not sure if S-o by me makes sense in that case.
pbs-api-types/src/datastore.rs | 3 +++
pbs-api-types/src/maintenance.rs | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index f6c255d3..888f5d5b 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -404,6 +404,9 @@ impl DataStoreConfig {
match current_type {
Some(MaintenanceType::ReadOnly) => { /* always OK */ }
Some(MaintenanceType::Offline) => { /* always OK */ }
+ Some(MaintenanceType::Unmount) => {
+ bail!("datastore is being unmounted");
+ }
Some(MaintenanceType::Delete) => {
match new_type {
Some(MaintenanceType::Delete) => { /* allow to delete a deleted storage */ }
diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 1e3413dc..fd4d3416 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -38,7 +38,6 @@ pub enum Operation {
/// Maintenance type.
pub enum MaintenanceType {
// TODO:
- // - Add "unmounting" once we got pluggable datastores
// - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
// operation, so that one can enable a mode where nothing new can be added but stuff can be
// cleaned
@@ -48,6 +47,8 @@ pub enum MaintenanceType {
Offline,
/// The datastore is being deleted.
Delete,
+ /// The (removable) datastore is being unmounted.
+ Unmount,
}
serde_plain::derive_display_from_serialize!(MaintenanceType);
serde_plain::derive_fromstr_from_deserialize!(MaintenanceType);
@@ -94,6 +95,8 @@ impl MaintenanceMode {
if let Some(Operation::Lookup) = operation {
return Ok(());
+ } else if self.ty == MaintenanceType::Unmount {
+ bail!("datastore is being unmounted");
} else if self.ty == MaintenanceType::Offline {
bail!("offline maintenance mode: {}", message);
} else if self.ty == MaintenanceType::ReadOnly {
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 05/26] disks: add UUID to partition info
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (3 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 04/26] maintenance: add 'Unmount' maintenance type Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-17 19:34 ` [pbs-devel] applied: " Thomas Lamprecht
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted Hannes Laimer
` (21 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/tools/disks/mod.rs | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 10c4eed0..9f47be36 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -57,6 +57,8 @@ pub struct LsblkInfo {
/// File system label.
#[serde(rename = "fstype")]
file_system_type: Option<String>,
+ /// File system UUID.
+ uuid: Option<String>,
}
impl DiskManage {
@@ -615,7 +617,7 @@ pub struct BlockDevStat {
/// Use lsblk to read partition type uuids and file system types.
pub fn get_lsblk_info() -> Result<Vec<LsblkInfo>, Error> {
let mut command = std::process::Command::new("lsblk");
- command.args(["--json", "-o", "path,parttype,fstype"]);
+ command.args(["--json", "-o", "path,parttype,fstype,uuid"]);
let output = proxmox_sys::command::run_command(command, None)?;
@@ -701,6 +703,8 @@ pub struct PartitionInfo {
pub size: Option<u64>,
/// GPT partition
pub gpt: bool,
+ /// UUID
+ pub uuid: Option<String>,
}
#[api(
@@ -891,8 +895,10 @@ fn get_partitions_info(
let mounted = disk.is_mounted().unwrap_or(false);
let mut filesystem = None;
+ let mut uuid = None;
if let Some(devpath) = devpath.as_ref() {
for info in lsblk_infos.iter().filter(|i| i.path.eq(devpath)) {
+ uuid = info.uuid.clone();
used = match info.partition_type.as_deref() {
Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
@@ -915,6 +921,7 @@ fn get_partitions_info(
filesystem,
size: disk.size().ok(),
gpt: disk.has_gpt(),
+ uuid,
}
})
.collect()
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (4 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 05/26] disks: add UUID to partition info Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 13:19 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 07/26] api: admin: add (un)mount endpoint for removable datastores Hannes Laimer
` (20 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
... at a specific location. This is removable datastore specific so it
takes both a uuid and mount location.
Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* clearify documentation
* make function more removable datastore specific to remove ambiguity
about what it does and what it is meant for
* only use for removable datastore
pbs-api-types/src/maintenance.rs | 2 +
pbs-datastore/src/datastore.rs | 73 +++++++++++++++++++++++++++++
pbs-datastore/src/lib.rs | 2 +-
src/server/metric_collection/mod.rs | 10 ++++
4 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index fd4d3416..9f51292e 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -82,6 +82,8 @@ impl MaintenanceMode {
/// task finishes, so all open files are closed.
pub fn is_offline(&self) -> bool {
self.ty == MaintenanceType::Offline
+ || self.ty == MaintenanceType::Unmount
+ || self.ty == MaintenanceType::Delete
}
pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index fb37bd5a..cadf9245 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1,5 +1,6 @@
use std::collections::{HashMap, HashSet};
use std::io::{self, Write};
+use std::os::unix::ffi::OsStrExt;
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
use std::sync::{Arc, LazyLock, Mutex};
@@ -14,6 +15,7 @@ use proxmox_schema::ApiType;
use proxmox_sys::error::SysError;
use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
+use proxmox_sys::linux::procfs::MountInfo;
use proxmox_sys::process_locker::ProcessLockSharedGuard;
use proxmox_worker_task::WorkerTaskContext;
@@ -46,6 +48,55 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
Ok(())
}
+/// Check if a device with a given UUID is currently mounted at store_mount_point by
+/// comparing the `st_rdev` values of `/dev/disk/by-uuid/<uuid>` and the source device in
+/// /proc/self/mountinfo.
+///
+/// If we can't check if it is mounted, we treat that as not mounted,
+/// returning false.
+///
+/// Reasons it could fail other than not being mounted where expected:
+/// - could not read /proc/self/mountinfo
+/// - could not stat /dev/disk/by-uuid/<uuid>
+/// - /dev/disk/by-uuid/<uuid> is not a block device
+///
+/// Since these are very much out of our control, there is no real value in distinguishing
+/// between them, so for this function they all are treated as 'device not mounted'
+pub fn is_datastore_mounted_at(store_mount_point: String, device_uuid: String) -> bool {
+ use nix::sys::stat::SFlag;
+
+ let store_mount_point = Path::new(&store_mount_point);
+
+ let dev_node = match nix::sys::stat::stat(format!("/dev/disk/by-uuid/{device_uuid}").as_str()) {
+ Ok(stat) if SFlag::from_bits_truncate(stat.st_mode) == SFlag::S_IFBLK => stat.st_rdev,
+ _ => return false,
+ };
+
+ let Ok(mount_info) = MountInfo::read() else {
+ return false;
+ };
+
+ for (_, entry) in mount_info {
+ let Some(source) = entry.mount_source else {
+ continue;
+ };
+
+ if entry.mount_point != store_mount_point || !source.as_bytes().starts_with(b"/") {
+ continue;
+ }
+
+ if let Ok(stat) = nix::sys::stat::stat(source.as_os_str()) {
+ let sflag = SFlag::from_bits_truncate(stat.st_mode);
+
+ if sflag == SFlag::S_IFBLK && stat.st_rdev == dev_node {
+ return true;
+ }
+ }
+ }
+
+ false
+}
+
/// Datastore Management
///
/// A Datastore can store severals backups, and provides the
@@ -154,6 +205,18 @@ impl DataStore {
bail!("datastore '{name}' is in {error}");
}
}
+ let mount_status = config
+ .get_mount_point()
+ .zip(config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+
+ if mount_status == Some(false) {
+ let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
+ datastore_cache.remove(&config.name);
+ bail!("Removable Datastore is not mounted");
+ }
let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
let entry = datastore_cache.get(name);
@@ -258,6 +321,16 @@ impl DataStore {
) -> Result<Arc<Self>, Error> {
let name = config.name.clone();
+ let mount_status = config
+ .get_mount_point()
+ .zip(config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+ if mount_status == Some(false) {
+ bail!("Datastore is not available")
+ }
+
let tuning: DatastoreTuning = serde_json::from_value(
DatastoreTuning::API_SCHEMA
.parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 202b0955..34113261 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -204,7 +204,7 @@ pub use manifest::BackupManifest;
pub use store_progress::StoreProgress;
mod datastore;
-pub use datastore::{check_backup_owner, DataStore};
+pub use datastore::{check_backup_owner, is_datastore_mounted_at, DataStore};
mod hierarchy;
pub use hierarchy::{
diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
index b95dba20..edba512c 100644
--- a/src/server/metric_collection/mod.rs
+++ b/src/server/metric_collection/mod.rs
@@ -176,6 +176,16 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DiskStat>) {
continue;
}
+ let mount_status = config
+ .get_mount_point()
+ .zip(config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ pbs_datastore::is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+ if mount_status == Some(false) {
+ continue;
+ }
+
datastores.push(gather_disk_stats(
disk_manager.clone(),
Path::new(&config.absolute_path()),
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted Hannes Laimer
@ 2024-11-21 13:19 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 13:19 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> ... at a specific location. This is removable datastore specific so it
> takes both a uuid and mount location.
>
> Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * clearify documentation
> * make function more removable datastore specific to remove ambiguity
> about what it does and what it is meant for
> * only use for removable datastore
>
> pbs-api-types/src/maintenance.rs | 2 +
> pbs-datastore/src/datastore.rs | 73 +++++++++++++++++++++++++++++
> pbs-datastore/src/lib.rs | 2 +-
> src/server/metric_collection/mod.rs | 10 ++++
> 4 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index fd4d3416..9f51292e 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -82,6 +82,8 @@ impl MaintenanceMode {
> /// task finishes, so all open files are closed.
> pub fn is_offline(&self) -> bool {
> self.ty == MaintenanceType::Offline
> + || self.ty == MaintenanceType::Unmount
> + || self.ty == MaintenanceType::Delete
> }
>
this part here doesn't really belong into this commit (it's not part of
the helper mentioned above). the comment and the (new) contents also
don't match - if the MaintenanceType is Delete (i.e., the datastore
contents are currently being deleted) then all open files can't be
already closed? same for Unmount - if it is currently being unmounted,
there might still be references open..
I think this should rather be explicit -> at the end of
unmounting/deletion, remove from cache? or this helper should be renamed
and the comment adapted, if it is actually not "is the datastore
offline" but "is it a removal candidate once all tasks have exited"..
> pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index fb37bd5a..cadf9245 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1,5 +1,6 @@
> use std::collections::{HashMap, HashSet};
> use std::io::{self, Write};
> +use std::os::unix::ffi::OsStrExt;
> use std::os::unix::io::AsRawFd;
> use std::path::{Path, PathBuf};
> use std::sync::{Arc, LazyLock, Mutex};
> @@ -14,6 +15,7 @@ use proxmox_schema::ApiType;
> use proxmox_sys::error::SysError;
> use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
> use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
> +use proxmox_sys::linux::procfs::MountInfo;
> use proxmox_sys::process_locker::ProcessLockSharedGuard;
> use proxmox_worker_task::WorkerTaskContext;
>
> @@ -46,6 +48,55 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
> Ok(())
> }
>
> +/// Check if a device with a given UUID is currently mounted at store_mount_point by
> +/// comparing the `st_rdev` values of `/dev/disk/by-uuid/<uuid>` and the source device in
> +/// /proc/self/mountinfo.
> +///
> +/// If we can't check if it is mounted, we treat that as not mounted,
> +/// returning false.
> +///
> +/// Reasons it could fail other than not being mounted where expected:
> +/// - could not read /proc/self/mountinfo
> +/// - could not stat /dev/disk/by-uuid/<uuid>
> +/// - /dev/disk/by-uuid/<uuid> is not a block device
> +///
> +/// Since these are very much out of our control, there is no real value in distinguishing
> +/// between them, so for this function they all are treated as 'device not mounted'
> +pub fn is_datastore_mounted_at(store_mount_point: String, device_uuid: String) -> bool {
> + use nix::sys::stat::SFlag;
> +
> + let store_mount_point = Path::new(&store_mount_point);
> +
> + let dev_node = match nix::sys::stat::stat(format!("/dev/disk/by-uuid/{device_uuid}").as_str()) {
> + Ok(stat) if SFlag::from_bits_truncate(stat.st_mode) == SFlag::S_IFBLK => stat.st_rdev,
> + _ => return false,
> + };
> +
> + let Ok(mount_info) = MountInfo::read() else {
> + return false;
> + };
> +
> + for (_, entry) in mount_info {
> + let Some(source) = entry.mount_source else {
> + continue;
> + };
> +
> + if entry.mount_point != store_mount_point || !source.as_bytes().starts_with(b"/") {
> + continue;
> + }
> +
> + if let Ok(stat) = nix::sys::stat::stat(source.as_os_str()) {
> + let sflag = SFlag::from_bits_truncate(stat.st_mode);
> +
> + if sflag == SFlag::S_IFBLK && stat.st_rdev == dev_node {
> + return true;
> + }
> + }
> + }
> +
> + false
> +}
> +
> /// Datastore Management
> ///
> /// A Datastore can store severals backups, and provides the
> @@ -154,6 +205,18 @@ impl DataStore {
> bail!("datastore '{name}' is in {error}");
> }
> }
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> +
> + if mount_status == Some(false) {
> + let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> + datastore_cache.remove(&config.name);
> + bail!("Removable Datastore is not mounted");
note the message here
> + }
>
> let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> let entry = datastore_cache.get(name);
> @@ -258,6 +321,16 @@ impl DataStore {
> ) -> Result<Arc<Self>, Error> {
> let name = config.name.clone();
>
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> + if mount_status == Some(false) {
> + bail!("Datastore is not available")
and here - shouldn't they be the same? could also be combined into a
helper (e.g., ensure_removable_datastore_is_mounted), if desired, there
is a third call site with the exact same code but yet another error
message in src/api2/admin/datastore.rs (and three more slight variations
where it's not (always) fatal that it is not mounted, but with the same
zip+map code before).
> + }
> +
> let tuning: DatastoreTuning = serde_json::from_value(
> DatastoreTuning::API_SCHEMA
> .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 202b0955..34113261 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -204,7 +204,7 @@ pub use manifest::BackupManifest;
> pub use store_progress::StoreProgress;
>
> mod datastore;
> -pub use datastore::{check_backup_owner, DataStore};
> +pub use datastore::{check_backup_owner, is_datastore_mounted_at, DataStore};
>
> mod hierarchy;
> pub use hierarchy::{
> diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
> index b95dba20..edba512c 100644
> --- a/src/server/metric_collection/mod.rs
> +++ b/src/server/metric_collection/mod.rs
> @@ -176,6 +176,16 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DiskStat>) {
> continue;
> }
>
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + pbs_datastore::is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> + if mount_status == Some(false) {
> + continue;
> + }
> +
this one of those variations.. since all call sites to
is_datastore_mounted_at start with the config, it might make more sense
to implement it there (or rather, using that as parameter, since we
don't want all that stat code in an api type ;)) and avoid all the
repetitive code?
then a simple get_datastore_mount_status(&config) -> Option(bool), and
maybe ensure_datastore_is_mounted(&config) -> Result<(), Error> would
solve the same issue, but be nicer to read?
> datastores.push(gather_disk_stats(
> disk_manager.clone(),
> Path::new(&config.absolute_path()),
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 07/26] api: admin: add (un)mount endpoint for removable datastores
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (5 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 13:35 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 08/26] api: removable datastore creation Hannes Laimer
` (19 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Removable datastores can be mounted unless
- they are already
- their device is not present
For unmounting the maintenance mode is set to `unmount`,
which prohibits the starting of any new tasks envolving any
IO, this mode is unset either
- on completion of the unmount
- on abort of the unmount tasks
If the unmounting itself should fail, the maintenance mode stays in
place and requires manual intervention by unsetting it in the config
file directly. This is intentional, as unmounting should not fail,
and if it should the situation should be looked at.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* allow multiple stores on one device
* add best effort attempt to unmount after failed creation
src/api2/admin/datastore.rs | 267 ++++++++++++++++++++++++++++++++++--
1 file changed, 257 insertions(+), 10 deletions(-)
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index b73ad0ff..a12262e7 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -3,7 +3,7 @@
use std::collections::HashSet;
use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
use std::sync::Arc;
use anyhow::{bail, format_err, Error};
@@ -13,7 +13,7 @@ use hyper::{header, Body, Response, StatusCode};
use serde::Deserialize;
use serde_json::{json, Value};
use tokio_stream::wrappers::ReceiverStream;
-use tracing::{info, warn};
+use tracing::{debug, info, warn};
use proxmox_async::blocking::WrappedReaderStream;
use proxmox_async::{io::AsyncChannelWriter, stream::AsyncReaderStream};
@@ -29,6 +29,7 @@ use proxmox_sys::fs::{
file_read_firstline, file_read_optional_string, replace_file, CreateOptions,
};
use proxmox_time::CalendarEvent;
+use proxmox_worker_task::WorkerTaskContext;
use pxar::accessor::aio::Accessor;
use pxar::EntryKind;
@@ -36,12 +37,12 @@ use pxar::EntryKind;
use pbs_api_types::{
print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus,
- GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, Operation,
- PruneJobOptions, SnapshotListItem, SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA,
- BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
- DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
- PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
- PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
+ GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode,
+ MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SnapshotVerifyState,
+ BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
+ BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH,
+ NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
+ PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
VERIFICATION_OUTDATED_AFTER_SCHEMA,
};
use pbs_client::pxar::{create_tar, create_zip};
@@ -57,8 +58,8 @@ use pbs_datastore::index::IndexFile;
use pbs_datastore::manifest::{BackupManifest, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
use pbs_datastore::prune::compute_prune_info;
use pbs_datastore::{
- check_backup_owner, task_tracking, BackupDir, BackupGroup, DataStore, LocalChunkReader,
- StoreProgress, CATALOG_NAME,
+ check_backup_owner, is_datastore_mounted_at, task_tracking, BackupDir, BackupGroup, DataStore,
+ LocalChunkReader, StoreProgress, CATALOG_NAME,
};
use pbs_tools::json::required_string_param;
use proxmox_rest_server::{formatter, WorkerTask};
@@ -2384,6 +2385,250 @@ pub async fn set_backup_owner(
.await?
}
+/// Here we
+///
+/// 1. mount the removable device to `<PBS_RUN_DIR>/mount/<RANDOM_UUID>`
+/// 2. bind mount `<PBS_RUN_DIR>/mount/<RANDOM_UUID>/<datastore.path>` to `/mnt/datastore/<datastore.name>`
+/// 3. unmount `<PBS_RUN_DIR>/mount/<RANDOM_UUID>`
+///
+/// leaving us with the datastore being mounted directly with its name under /mnt/datastore/...
+///
+/// The reason for the randomized device mounting paths is to avoid two tasks trying to mount to
+/// the same path, this is *very* unlikely since the device is only mounted really shortly, but
+/// technically possible.
+pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
+ if let (Some(uuid), Some(mount_point)) = (
+ datastore.backing_device.as_ref(),
+ datastore.get_mount_point(),
+ ) {
+ if pbs_datastore::is_datastore_mounted_at(mount_point.clone(), uuid.to_string()) {
+ bail!("device is already mounted at '{}'", mount_point);
+ }
+ let tmp_mount_path = format!(
+ "{}/{:x}",
+ pbs_buildcfg::rundir!("/mount"),
+ proxmox_uuid::Uuid::generate()
+ );
+
+ let default_options = proxmox_sys::fs::CreateOptions::new();
+ proxmox_sys::fs::create_path(
+ &tmp_mount_path,
+ Some(default_options.clone()),
+ Some(default_options.clone()),
+ )?;
+
+ debug!("mounting '{uuid}' to '{}'", tmp_mount_path);
+ crate::tools::disks::mount_by_uuid(uuid, Path::new(&tmp_mount_path))?;
+
+ let full_store_path = format!(
+ "{tmp_mount_path}/{}",
+ datastore.path.trim_start_matches('/')
+ );
+ let backup_user = pbs_config::backup_user()?;
+ let options = CreateOptions::new()
+ .owner(backup_user.uid)
+ .group(backup_user.gid);
+
+ proxmox_sys::fs::create_path(
+ &mount_point,
+ Some(default_options.clone()),
+ Some(options.clone()),
+ )?;
+
+ // can't be created before it is mounted, so we have to do it here
+ proxmox_sys::fs::create_path(
+ &full_store_path,
+ Some(default_options.clone()),
+ Some(options.clone()),
+ )?;
+
+ info!(
+ "mounting '{}'({}) to '{}'",
+ datastore.name, datastore.path, mount_point
+ );
+ if let Err(err) =
+ crate::tools::disks::bind_mount(Path::new(&full_store_path), Path::new(&mount_point))
+ {
+ debug!("unmounting '{}'", tmp_mount_path);
+ let _ = crate::tools::disks::unmount_by_mountpoint(Path::new(&tmp_mount_path));
+ let _ = std::fs::remove_dir(std::path::Path::new(&tmp_mount_path));
+ return Err(format_err!(
+ "Datastore '{}' cound not be mounted: {}.",
+ datastore.name,
+ err
+ ));
+ }
+
+ debug!("unmounting '{}'", tmp_mount_path);
+ crate::tools::disks::unmount_by_mountpoint(Path::new(&tmp_mount_path))?;
+ std::fs::remove_dir(std::path::Path::new(&tmp_mount_path))?;
+
+ Ok(())
+ } else {
+ Err(format_err!(
+ "Datastore '{}' cannot be mounted because it is not removable.",
+ datastore.name
+ ))
+ }
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ store: {
+ schema: DATASTORE_SCHEMA,
+ },
+ }
+ },
+ returns: {
+ schema: UPID_SCHEMA,
+ },
+ access: {
+ permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
+ },
+)]
+/// Mount removable datastore.
+pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let (section_config, _digest) = pbs_config::datastore::config()?;
+ let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
+
+ if datastore.backing_device.is_none() {
+ bail!("datastore '{store}' is not removable");
+ }
+
+ let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+ let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+ let upid = WorkerTask::new_thread(
+ "mount-device",
+ Some(store),
+ auth_id.to_string(),
+ to_stdout,
+ move |_worker| do_mount_device(datastore),
+ )?;
+
+ Ok(json!(upid))
+}
+
+fn unset_unmount_maintenance(store: &str) -> Result<(), Error> {
+ let _lock = pbs_config::datastore::lock_config()?;
+ let (mut section_config, _digest) = pbs_config::datastore::config()?;
+ let mut store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
+ if store_config
+ .get_maintenance_mode()
+ .map_or(true, |m| m.ty != MaintenanceType::Unmount)
+ {
+ bail!("Maintenance mode should have been 'Unmount'")
+ }
+ store_config.maintenance_mode = None;
+ section_config.set_data(store, "datastore", &store_config)?;
+ pbs_config::datastore::save_config(§ion_config)?;
+ Ok(())
+}
+
+fn do_unmount_device(
+ datastore: DataStoreConfig,
+ worker: Option<&dyn WorkerTaskContext>,
+) -> Result<(), Error> {
+ let mut active_operations = task_tracking::get_active_operations(&datastore.name)?;
+ let mut old_status = String::new();
+ while active_operations.read + active_operations.write > 0 {
+ if let Some(worker) = worker {
+ if worker.abort_requested() {
+ unset_unmount_maintenance(&datastore.name)?;
+ bail!("aborted, due to user request");
+ }
+ let status = format!(
+ "cannot unmount yet, still {} read and {} write operations active",
+ active_operations.read, active_operations.write
+ );
+ if status != old_status {
+ info!("{status}");
+ old_status = status;
+ }
+ }
+ std::thread::sleep(std::time::Duration::from_secs(1));
+ active_operations = task_tracking::get_active_operations(&datastore.name)?;
+ }
+ if let Some(mount_point) = datastore.get_mount_point() {
+ crate::tools::disks::unmount_by_mountpoint(Path::new(&mount_point))?;
+ unset_unmount_maintenance(&datastore.name)?;
+ }
+ Ok(())
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ store: { schema: DATASTORE_SCHEMA },
+ },
+ },
+ returns: {
+ schema: UPID_SCHEMA,
+ },
+ access: {
+ permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
+ }
+)]
+/// Unmount a removable device that is associated with the datastore
+pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let _lock = pbs_config::datastore::lock_config()?;
+ let (mut section_config, _digest) = pbs_config::datastore::config()?;
+ let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
+
+ if datastore.backing_device.is_none() {
+ bail!("datastore '{store}' is not removable");
+ }
+
+ let mount_status = datastore
+ .get_mount_point()
+ .zip(datastore.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+
+ if mount_status == Some(false) {
+ bail!("datastore '{store}' is not mounted");
+ }
+
+ datastore.set_maintenance_mode(Some(MaintenanceMode {
+ ty: MaintenanceType::Unmount,
+ message: None,
+ }))?;
+ section_config.set_data(&store, "datastore", &datastore)?;
+ pbs_config::datastore::save_config(§ion_config)?;
+
+ drop(_lock);
+
+ let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+ let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+ if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
+ {
+ let sock = proxmox_daemon::command_socket::path_from_pid(proxy_pid);
+ let _ = proxmox_daemon::command_socket::send_raw(
+ sock,
+ &format!(
+ "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
+ &store
+ ),
+ )
+ .await;
+ }
+
+ let upid = WorkerTask::new_thread(
+ "unmount-device",
+ Some(store),
+ auth_id.to_string(),
+ to_stdout,
+ move |worker| do_unmount_device(datastore, Some(&worker)),
+ )?;
+
+ Ok(json!(upid))
+}
+
#[sortable]
const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
(
@@ -2422,6 +2667,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
.get(&API_METHOD_LIST_GROUPS)
.delete(&API_METHOD_DELETE_GROUP),
),
+ ("mount", &Router::new().post(&API_METHOD_MOUNT)),
(
"namespace",
// FIXME: move into datastore:: sub-module?!
@@ -2456,6 +2702,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
.delete(&API_METHOD_DELETE_SNAPSHOT),
),
("status", &Router::new().get(&API_METHOD_STATUS)),
+ ("unmount", &Router::new().post(&API_METHOD_UNMOUNT)),
(
"upload-backup-log",
&Router::new().upload(&API_METHOD_UPLOAD_BACKUP_LOG),
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 07/26] api: admin: add (un)mount endpoint for removable datastores
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 07/26] api: admin: add (un)mount endpoint for removable datastores Hannes Laimer
@ 2024-11-21 13:35 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 13:35 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Removable datastores can be mounted unless
> - they are already
> - their device is not present
> For unmounting the maintenance mode is set to `unmount`,
> which prohibits the starting of any new tasks envolving any
> IO, this mode is unset either
> - on completion of the unmount
> - on abort of the unmount tasks
> If the unmounting itself should fail, the maintenance mode stays in
> place and requires manual intervention by unsetting it in the config
> file directly. This is intentional, as unmounting should not fail,
> and if it should the situation should be looked at.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * allow multiple stores on one device
> * add best effort attempt to unmount after failed creation
>
> src/api2/admin/datastore.rs | 267 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 257 insertions(+), 10 deletions(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index b73ad0ff..a12262e7 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -3,7 +3,7 @@
> use std::collections::HashSet;
> use std::ffi::OsStr;
> use std::os::unix::ffi::OsStrExt;
> -use std::path::PathBuf;
> +use std::path::{Path, PathBuf};
> use std::sync::Arc;
>
> use anyhow::{bail, format_err, Error};
> @@ -13,7 +13,7 @@ use hyper::{header, Body, Response, StatusCode};
> use serde::Deserialize;
> use serde_json::{json, Value};
> use tokio_stream::wrappers::ReceiverStream;
> -use tracing::{info, warn};
> +use tracing::{debug, info, warn};
>
> use proxmox_async::blocking::WrappedReaderStream;
> use proxmox_async::{io::AsyncChannelWriter, stream::AsyncReaderStream};
> @@ -29,6 +29,7 @@ use proxmox_sys::fs::{
> file_read_firstline, file_read_optional_string, replace_file, CreateOptions,
> };
> use proxmox_time::CalendarEvent;
> +use proxmox_worker_task::WorkerTaskContext;
>
> use pxar::accessor::aio::Accessor;
> use pxar::EntryKind;
> @@ -36,12 +37,12 @@ use pxar::EntryKind;
> use pbs_api_types::{
> print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
> Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus,
> - GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, Operation,
> - PruneJobOptions, SnapshotListItem, SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA,
> - BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
> - DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
> - PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
> - PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
> + GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode,
> + MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SnapshotVerifyState,
> + BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
> + BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH,
> + NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
> + PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
> VERIFICATION_OUTDATED_AFTER_SCHEMA,
> };
> use pbs_client::pxar::{create_tar, create_zip};
> @@ -57,8 +58,8 @@ use pbs_datastore::index::IndexFile;
> use pbs_datastore::manifest::{BackupManifest, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
> use pbs_datastore::prune::compute_prune_info;
> use pbs_datastore::{
> - check_backup_owner, task_tracking, BackupDir, BackupGroup, DataStore, LocalChunkReader,
> - StoreProgress, CATALOG_NAME,
> + check_backup_owner, is_datastore_mounted_at, task_tracking, BackupDir, BackupGroup, DataStore,
> + LocalChunkReader, StoreProgress, CATALOG_NAME,
> };
> use pbs_tools::json::required_string_param;
> use proxmox_rest_server::{formatter, WorkerTask};
> @@ -2384,6 +2385,250 @@ pub async fn set_backup_owner(
> .await?
> }
>
> +/// Here we
> +///
> +/// 1. mount the removable device to `<PBS_RUN_DIR>/mount/<RANDOM_UUID>`
> +/// 2. bind mount `<PBS_RUN_DIR>/mount/<RANDOM_UUID>/<datastore.path>` to `/mnt/datastore/<datastore.name>`
> +/// 3. unmount `<PBS_RUN_DIR>/mount/<RANDOM_UUID>`
> +///
> +/// leaving us with the datastore being mounted directly with its name under /mnt/datastore/...
> +///
> +/// The reason for the randomized device mounting paths is to avoid two tasks trying to mount to
> +/// the same path, this is *very* unlikely since the device is only mounted really shortly, but
> +/// technically possible.
> +pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
> + if let (Some(uuid), Some(mount_point)) = (
> + datastore.backing_device.as_ref(),
> + datastore.get_mount_point(),
> + ) {
another variation, see previous patch comments..
> + if pbs_datastore::is_datastore_mounted_at(mount_point.clone(), uuid.to_string()) {
> + bail!("device is already mounted at '{}'", mount_point);
> + }
> + let tmp_mount_path = format!(
> + "{}/{:x}",
> + pbs_buildcfg::rundir!("/mount"),
> + proxmox_uuid::Uuid::generate()
> + );
> +
> + let default_options = proxmox_sys::fs::CreateOptions::new();
> + proxmox_sys::fs::create_path(
> + &tmp_mount_path,
> + Some(default_options.clone()),
> + Some(default_options.clone()),
> + )?;
> +
> + debug!("mounting '{uuid}' to '{}'", tmp_mount_path);
IMHO this could be info!, we are in a task context here with very little
output, and if something went wrong, the extra info can only help..
maybe add in a "temporarily" at the beginning..
> + crate::tools::disks::mount_by_uuid(uuid, Path::new(&tmp_mount_path))?;
because else, if this fails, the user has no idea what's going on unless
they happen to run in debug mode..
> +
> + let full_store_path = format!(
> + "{tmp_mount_path}/{}",
> + datastore.path.trim_start_matches('/')
> + );
> + let backup_user = pbs_config::backup_user()?;
> + let options = CreateOptions::new()
> + .owner(backup_user.uid)
> + .group(backup_user.gid);
> +
> + proxmox_sys::fs::create_path(
> + &mount_point,
> + Some(default_options.clone()),
> + Some(options.clone()),
> + )?;
should we add some context to the error here?
> +
> + // can't be created before it is mounted, so we have to do it here
> + proxmox_sys::fs::create_path(
> + &full_store_path,
> + Some(default_options.clone()),
> + Some(options.clone()),
> + )?;
and here?
> +
> + info!(
> + "mounting '{}'({}) to '{}'",
> + datastore.name, datastore.path, mount_point
> + );
if the message above becomes info, then this should probably say
something like "bind mounting '{full_store_path}' to '{mount_point}'"
> + if let Err(err) =
> + crate::tools::disks::bind_mount(Path::new(&full_store_path), Path::new(&mount_point))
> + {
> + debug!("unmounting '{}'", tmp_mount_path);
> + let _ = crate::tools::disks::unmount_by_mountpoint(Path::new(&tmp_mount_path));
should we log errors her?
> + let _ = std::fs::remove_dir(std::path::Path::new(&tmp_mount_path));
and here? if those fail, we might need additional cleanup?
> + return Err(format_err!(
> + "Datastore '{}' cound not be mounted: {}.",
> + datastore.name,
> + err
> + ));
> + }
> +
> + debug!("unmounting '{}'", tmp_mount_path);
if the first message becomes info, this should too (and maybe add in
that the path being unmounted was temporary).
> + crate::tools::disks::unmount_by_mountpoint(Path::new(&tmp_mount_path))?;
> + std::fs::remove_dir(std::path::Path::new(&tmp_mount_path))?;
error context here might be nice as well
> +
> + Ok(())
> + } else {
> + Err(format_err!(
> + "Datastore '{}' cannot be mounted because it is not removable.",
> + datastore.name
> + ))
> + }
> +}
> +
> +#[api(
> + protected: true,
> + input: {
> + properties: {
> + store: {
> + schema: DATASTORE_SCHEMA,
> + },
> + }
> + },
> + returns: {
> + schema: UPID_SCHEMA,
> + },
> + access: {
> + permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> + },
> +)]
> +/// Mount removable datastore.
> +pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> + let (section_config, _digest) = pbs_config::datastore::config()?;
> + let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> + if datastore.backing_device.is_none() {
> + bail!("datastore '{store}' is not removable");
> + }
> +
> + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> + let upid = WorkerTask::new_thread(
> + "mount-device",
> + Some(store),
> + auth_id.to_string(),
> + to_stdout,
> + move |_worker| do_mount_device(datastore),
> + )?;
> +
> + Ok(json!(upid))
> +}
> +
> +fn unset_unmount_maintenance(store: &str) -> Result<(), Error> {
> + let _lock = pbs_config::datastore::lock_config()?;
> + let (mut section_config, _digest) = pbs_config::datastore::config()?;
> + let mut store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
> + if store_config
> + .get_maintenance_mode()
> + .map_or(true, |m| m.ty != MaintenanceType::Unmount)
> + {
> + bail!("Maintenance mode should have been 'Unmount'")
> + }
> + store_config.maintenance_mode = None;
> + section_config.set_data(store, "datastore", &store_config)?;
> + pbs_config::datastore::save_config(§ion_config)?;
> + Ok(())
> +}
> +
> +fn do_unmount_device(
> + datastore: DataStoreConfig,
> + worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> + let mut active_operations = task_tracking::get_active_operations(&datastore.name)?;
> + let mut old_status = String::new();
> + while active_operations.read + active_operations.write > 0 {
> + if let Some(worker) = worker {
> + if worker.abort_requested() {
> + unset_unmount_maintenance(&datastore.name)?;
this error should be caught and converted to a warning
> + bail!("aborted, due to user request");
else this much more important information might not be printed
> + }
this check should also be done below the loop, else this is racy..
> + let status = format!(
> + "cannot unmount yet, still {} read and {} write operations active",
this reads a bit strange language-wise, maybe it can be rephrased?
unmounting not possible yet, there are still ..
> + active_operations.read, active_operations.write
> + );
> + if status != old_status {
> + info!("{status}");
> + old_status = status;
> + }
> + }
> + std::thread::sleep(std::time::Duration::from_secs(1));
> + active_operations = task_tracking::get_active_operations(&datastore.name)?;
> + }
> + if let Some(mount_point) = datastore.get_mount_point() {
> + crate::tools::disks::unmount_by_mountpoint(Path::new(&mount_point))?;
> + unset_unmount_maintenance(&datastore.name)?;
so if I clear the maintenance mode, it will get unmounted anyway, and
only then tell me that the maintenance mode is unexpected? this should
re-lock and read the config before unmounting..
that likely means you actually want the helper above to give you the
lock and check the state, and then have a second helper to unset it and
write the config out (if we had proper locked configs as types this
would be easier :()..
> + }
> + Ok(())
> +}
> +
> +#[api(
> + protected: true,
> + input: {
> + properties: {
> + store: { schema: DATASTORE_SCHEMA },
> + },
> + },
> + returns: {
> + schema: UPID_SCHEMA,
> + },
> + access: {
> + permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> + }
> +)]
> +/// Unmount a removable device that is associated with the datastore
> +pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> + let _lock = pbs_config::datastore::lock_config()?;
> + let (mut section_config, _digest) = pbs_config::datastore::config()?;
> + let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> + if datastore.backing_device.is_none() {
> + bail!("datastore '{store}' is not removable");
> + }
> +
> + let mount_status = datastore
> + .get_mount_point()
> + .zip(datastore.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
another variant ;)
> +
> + if mount_status == Some(false) {
> + bail!("datastore '{store}' is not mounted");
> + }
> +
> + datastore.set_maintenance_mode(Some(MaintenanceMode {
> + ty: MaintenanceType::Unmount,
> + message: None,
> + }))?;
> + section_config.set_data(&store, "datastore", &datastore)?;
> + pbs_config::datastore::save_config(§ion_config)?;
> +
> + drop(_lock);
> +
> + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> + if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> + {
> + let sock = proxmox_daemon::command_socket::path_from_pid(proxy_pid);
> + let _ = proxmox_daemon::command_socket::send_raw(
> + sock,
> + &format!(
> + "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
> + &store
> + ),
> + )
> + .await;
> + }
> +
> + let upid = WorkerTask::new_thread(
> + "unmount-device",
> + Some(store),
> + auth_id.to_string(),
> + to_stdout,
> + move |worker| do_unmount_device(datastore, Some(&worker)),
> + )?;
> +
> + Ok(json!(upid))
> +}
> +
> #[sortable]
> const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
> (
> @@ -2422,6 +2667,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
> .get(&API_METHOD_LIST_GROUPS)
> .delete(&API_METHOD_DELETE_GROUP),
> ),
> + ("mount", &Router::new().post(&API_METHOD_MOUNT)),
> (
> "namespace",
> // FIXME: move into datastore:: sub-module?!
> @@ -2456,6 +2702,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
> .delete(&API_METHOD_DELETE_SNAPSHOT),
> ),
> ("status", &Router::new().get(&API_METHOD_STATUS)),
> + ("unmount", &Router::new().post(&API_METHOD_UNMOUNT)),
> (
> "upload-backup-log",
> &Router::new().upload(&API_METHOD_UPLOAD_BACKUP_LOG),
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 08/26] api: removable datastore creation
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (6 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 07/26] api: admin: add (un)mount endpoint for removable datastores Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:22 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem Hannes Laimer
` (18 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Devices can contains multiple datastores, the only limitations is that
they are not allowed to be nested.
If the specified path already contains a datastore, `reuse datastore` has
to be set so it'll be added without creating a chunckstore.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* use recently added 'reuse datastore'
* allow creation even if device is already used by datastore, just no
nesting
src/api2/config/datastore.rs | 50 +++++++++++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 6 deletions(-)
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 374c302f..9140a7a4 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -20,7 +20,8 @@ use pbs_config::BackupLockGuard;
use pbs_datastore::chunk_store::ChunkStore;
use crate::api2::admin::{
- prune::list_prune_jobs, sync::list_sync_jobs, verify::list_verification_jobs,
+ datastore::do_mount_device, prune::list_prune_jobs, sync::list_sync_jobs,
+ verify::list_verification_jobs,
};
use crate::api2::config::prune::{delete_prune_job, do_create_prune_job};
use crate::api2::config::sync::delete_sync_job;
@@ -31,6 +32,7 @@ use pbs_config::CachedUserInfo;
use proxmox_rest_server::WorkerTask;
use crate::server::jobstate;
+use crate::tools::disks::unmount_by_mountpoint;
#[api(
input: {
@@ -72,7 +74,11 @@ pub(crate) fn do_create_datastore(
datastore: DataStoreConfig,
reuse_datastore: bool,
) -> Result<(), Error> {
- let path: PathBuf = datastore.path.clone().into();
+ let path: PathBuf = datastore.absolute_path().into();
+ let need_unmount = datastore.get_mount_point().is_some() && {
+ do_mount_device(datastore.clone())?;
+ true
+ };
if path.parent().is_none() {
bail!("cannot create datastore in root path");
@@ -84,24 +90,32 @@ pub(crate) fn do_create_datastore(
)?;
if reuse_datastore {
- ChunkStore::verify_chunkstore(&path)?;
+ if let Err(e) = ChunkStore::verify_chunkstore(&path) {
+ let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
+ return Err(e);
+ }
} else {
if let Ok(dir) = std::fs::read_dir(&path) {
for file in dir {
let name = file?.file_name();
if !name.to_str().map_or(false, |name| name.starts_with('.')) {
+ let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
bail!("datastore path is not empty");
}
}
}
let backup_user = pbs_config::backup_user()?;
- let _store = ChunkStore::create(
+ let res = ChunkStore::create(
&datastore.name,
- path,
+ path.clone(),
backup_user.uid,
backup_user.gid,
tuning.sync_level.unwrap_or_default(),
- )?;
+ );
+ if let Err(e) = res {
+ let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
+ return Err(e);
+ }
}
config.set_data(&datastore.name, "datastore", &datastore)?;
@@ -145,6 +159,30 @@ pub fn create_datastore(
param_bail!("name", "datastore '{}' already exists.", config.name);
}
+ if !config.path.starts_with("/") {
+ param_bail!("path", "expected an abolute path, '{}' is not", config.path);
+ }
+
+ if let Some(uuid) = &config.backing_device {
+ for (store_name, (_, store_config)) in §ion_config.sections {
+ if let (Some(store_uuid), Some(store_path)) = (
+ store_config["backing-device"].as_str(),
+ store_config["path"].as_str(),
+ ) {
+ // We don't allow two datastores to be nested in each other, so if
+ // ds1: /a/b -> can't create new one at /, /a or /a/b/..., /a/c is fine
+ if store_uuid == uuid
+ && (store_path.starts_with(&config.path) || config.path.starts_with(store_path))
+ {
+ param_bail!(
+ "path",
+ "can't nest datastores, '{store_name}' already in '{store_path}'",
+ );
+ }
+ };
+ }
+ }
+
let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 08/26] api: removable datastore creation
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 08/26] api: removable datastore creation Hannes Laimer
@ 2024-11-21 14:22 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:22 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Devices can contains multiple datastores, the only limitations is that
> they are not allowed to be nested.
> If the specified path already contains a datastore, `reuse datastore` has
> to be set so it'll be added without creating a chunckstore.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * use recently added 'reuse datastore'
> * allow creation even if device is already used by datastore, just no
> nesting
>
> src/api2/config/datastore.rs | 50 +++++++++++++++++++++++++++++++-----
> 1 file changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 374c302f..9140a7a4 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -20,7 +20,8 @@ use pbs_config::BackupLockGuard;
> use pbs_datastore::chunk_store::ChunkStore;
>
> use crate::api2::admin::{
> - prune::list_prune_jobs, sync::list_sync_jobs, verify::list_verification_jobs,
> + datastore::do_mount_device, prune::list_prune_jobs, sync::list_sync_jobs,
> + verify::list_verification_jobs,
> };
> use crate::api2::config::prune::{delete_prune_job, do_create_prune_job};
> use crate::api2::config::sync::delete_sync_job;
> @@ -31,6 +32,7 @@ use pbs_config::CachedUserInfo;
> use proxmox_rest_server::WorkerTask;
>
> use crate::server::jobstate;
> +use crate::tools::disks::unmount_by_mountpoint;
>
> #[api(
> input: {
> @@ -72,7 +74,11 @@ pub(crate) fn do_create_datastore(
> datastore: DataStoreConfig,
> reuse_datastore: bool,
> ) -> Result<(), Error> {
> - let path: PathBuf = datastore.path.clone().into();
> + let path: PathBuf = datastore.absolute_path().into();
> + let need_unmount = datastore.get_mount_point().is_some() && {
nit: would be easier to read as
let need_unmount = ;
if need_unmount {do_mount_device(..)?; }
> + do_mount_device(datastore.clone())?;
> + true
> + };
>
> if path.parent().is_none() {
> bail!("cannot create datastore in root path");
this can fail (well, not really for a removable datastore), but also
some parsing code between this
> @@ -84,24 +90,32 @@ pub(crate) fn do_create_datastore(
and this, and this repeats below as well..
it might be better to wrap most of the body after the mounting, check
for any error, then do the cleanup/unmounting in one place?
> )?;
>
> if reuse_datastore {
> - ChunkStore::verify_chunkstore(&path)?;
> + if let Err(e) = ChunkStore::verify_chunkstore(&path) {
> + let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
> + return Err(e);
> + }
then this
> } else {
> if let Ok(dir) = std::fs::read_dir(&path) {
> for file in dir {
> let name = file?.file_name();
> if !name.to_str().map_or(false, |name| name.starts_with('.')) {
> + let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
> bail!("datastore path is not empty");
and this
> }
> }
> }
> let backup_user = pbs_config::backup_user()?;
> - let _store = ChunkStore::create(
> + let res = ChunkStore::create(
> &datastore.name,
> - path,
> + path.clone(),
> backup_user.uid,
> backup_user.gid,
> tuning.sync_level.unwrap_or_default(),
> - )?;
> + );
> + if let Err(e) = res {
> + let _ = need_unmount && unmount_by_mountpoint(&path).is_ok();
and this could all just return/bubble up the error, and the cleanup
logic lives on call level higher..
> + return Err(e);
> + }
> }
>
> config.set_data(&datastore.name, "datastore", &datastore)?;
> @@ -145,6 +159,30 @@ pub fn create_datastore(
> param_bail!("name", "datastore '{}' already exists.", config.name);
> }
>
> + if !config.path.starts_with("/") {
> + param_bail!("path", "expected an abolute path, '{}' is not", config.path);
> + }
but the schema is now updated to allow relative paths for removable
datastores? doesn't this need another condition to only apply for
removable datastores? I guess this was only tested via the
create_datastore_disk code path, which calls do_create_datastore
directly, and not this API endpoint..
> +
> + if let Some(uuid) = &config.backing_device {
but this here should apply to all datastores? it causes GC confusion
also for regular ones if they get nested.. and since this only affects
attempts to create datastores, it should be okay to make it fatal?
> + for (store_name, (_, store_config)) in §ion_config.sections {
> + if let (Some(store_uuid), Some(store_path)) = (
> + store_config["backing-device"].as_str(),
> + store_config["path"].as_str(),
> + ) {
> + // We don't allow two datastores to be nested in each other, so if
> + // ds1: /a/b -> can't create new one at /, /a or /a/b/..., /a/c is fine
> + if store_uuid == uuid
> + && (store_path.starts_with(&config.path) || config.path.starts_with(store_path))
> + {
> + param_bail!(
> + "path",
> + "can't nest datastores, '{store_name}' already in '{store_path}'",
"nested datastores not allowed: "
is a bit easier/nicer to read I think
> + );
> + }
> + };
> + }
> + }
> +
> let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (7 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 08/26] api: removable datastore creation Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:27 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 10/26] bin: manager: add (un)mount command Hannes Laimer
` (17 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Only removable datastores have a mount status, so normal ones will have
`None`, and for removable ones it is either mounted (`Some(true)`) or
not mounted (`Some(false)`).
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* replace is_availabl+removable field combo, with single mount_status
field
pbs-api-types/src/datastore.rs | 9 ++++++++-
src/api2/admin/datastore.rs | 22 ++++++++++++++--------
src/api2/status/mod.rs | 29 +++++++++++++++++++++++++----
3 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 888f5d5b..e111d692 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -454,6 +454,9 @@ impl DataStoreConfig {
pub struct DataStoreListItem {
pub store: String,
pub comment: Option<String>,
+ /// Is datastore mounted, None for not-removable datastores
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub mount_status: Option<bool>,
/// If the datastore is in maintenance mode, information about it
#[serde(skip_serializing_if = "Option::is_none")]
pub maintenance: Option<String>,
@@ -1453,6 +1456,9 @@ pub struct DataStoreStatusListItem {
/// The available bytes of the underlying storage. (-1 on error)
#[serde(skip_serializing_if = "Option::is_none")]
pub avail: Option<u64>,
+ /// The datastore is mounted, None for not-removable datastores
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub mount_status: Option<bool>,
/// A list of usages of the past (last Month).
#[serde(skip_serializing_if = "Option::is_none")]
pub history: Option<Vec<Option<f64>>>,
@@ -1477,12 +1483,13 @@ pub struct DataStoreStatusListItem {
}
impl DataStoreStatusListItem {
- pub fn empty(store: &str, err: Option<String>) -> Self {
+ pub fn empty(store: &str, err: Option<String>, mount_status: Option<bool>) -> Self {
DataStoreStatusListItem {
store: store.to_owned(),
total: None,
used: None,
avail: None,
+ mount_status,
history: None,
history_start: None,
history_delta: None,
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a12262e7..a9d9040f 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1310,8 +1310,8 @@ pub fn get_datastore_list(
let mut list = Vec::new();
- for (store, (_, data)) in &config.sections {
- let acl_path = &["datastore", store];
+ for (store, (_, data)) in config.sections {
+ let acl_path = &["datastore", &store];
let user_privs = user_info.lookup_privs(&auth_id, acl_path);
let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
@@ -1322,15 +1322,21 @@ pub fn get_datastore_list(
}
}
+ let store_config: DataStoreConfig = serde_json::from_value(data)?;
+
+ let mount_status = store_config
+ .get_mount_point()
+ .zip(store_config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+
if allowed || allow_id {
list.push(DataStoreListItem {
store: store.clone(),
- comment: if !allowed {
- None
- } else {
- data["comment"].as_str().map(String::from)
- },
- maintenance: data["maintenance-mode"].as_str().map(String::from),
+ comment: store_config.comment.filter(|_| allowed),
+ mount_status,
+ maintenance: store_config.maintenance_mode,
});
}
}
diff --git a/src/api2/status/mod.rs b/src/api2/status/mod.rs
index 113aa985..508331fe 100644
--- a/src/api2/status/mod.rs
+++ b/src/api2/status/mod.rs
@@ -10,11 +10,12 @@ use proxmox_schema::api;
use proxmox_sortable_macro::sortable;
use pbs_api_types::{
- Authid, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+ Authid, DataStoreConfig, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT,
+ PRIV_DATASTORE_BACKUP,
};
use pbs_config::CachedUserInfo;
-use pbs_datastore::DataStore;
+use pbs_datastore::{is_datastore_mounted_at, DataStore};
use crate::server::metric_collection::rrd::extract_rrd_data;
use crate::tools::statistics::linear_regression;
@@ -51,10 +52,25 @@ pub async fn datastore_status(
for (store, (_, _)) in &config.sections {
let user_privs = user_info.lookup_privs(&auth_id, &["datastore", store]);
let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
+
+ let store_config = config.lookup::<DataStoreConfig>("datastore", store)?;
+
+ let mount_status = store_config
+ .get_mount_point()
+ .zip(store_config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+
+ if let Some(false) = mount_status {
+ list.push(DataStoreStatusListItem::empty(store, None, mount_status));
+ continue;
+ }
+
if !allowed {
if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
if can_access_any_namespace(datastore, &auth_id, &user_info) {
- list.push(DataStoreStatusListItem::empty(store, None));
+ list.push(DataStoreStatusListItem::empty(store, None, mount_status));
}
}
continue;
@@ -63,7 +79,11 @@ pub async fn datastore_status(
let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
Ok(datastore) => datastore,
Err(err) => {
- list.push(DataStoreStatusListItem::empty(store, Some(err.to_string())));
+ list.push(DataStoreStatusListItem::empty(
+ store,
+ Some(err.to_string()),
+ mount_status,
+ ));
continue;
}
};
@@ -74,6 +94,7 @@ pub async fn datastore_status(
total: Some(status.total),
used: Some(status.used),
avail: Some(status.available),
+ mount_status,
history: None,
history_start: None,
history_delta: None,
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem Hannes Laimer
@ 2024-11-21 14:27 ` Fabian Grünbichler
2024-11-21 14:41 ` Hannes Laimer
0 siblings, 1 reply; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:27 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Only removable datastores have a mount status, so normal ones will have
> `None`, and for removable ones it is either mounted (`Some(true)`) or
> not mounted (`Some(false)`).
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * replace is_availabl+removable field combo, with single mount_status
> field
>
> pbs-api-types/src/datastore.rs | 9 ++++++++-
> src/api2/admin/datastore.rs | 22 ++++++++++++++--------
> src/api2/status/mod.rs | 29 +++++++++++++++++++++++++----
> 3 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 888f5d5b..e111d692 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -454,6 +454,9 @@ impl DataStoreConfig {
> pub struct DataStoreListItem {
> pub store: String,
> pub comment: Option<String>,
> + /// Is datastore mounted, None for not-removable datastores
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub mount_status: Option<bool>,
Option<bool> is okay for internal usage, but in an api type, wouldn't a
proper enum be nicer?
NonRemovable, Mounted, NotMounted
> /// If the datastore is in maintenance mode, information about it
> #[serde(skip_serializing_if = "Option::is_none")]
> pub maintenance: Option<String>,
> @@ -1453,6 +1456,9 @@ pub struct DataStoreStatusListItem {
> /// The available bytes of the underlying storage. (-1 on error)
> #[serde(skip_serializing_if = "Option::is_none")]
> pub avail: Option<u64>,
> + /// The datastore is mounted, None for not-removable datastores
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub mount_status: Option<bool>,
Option<bool> is okay for internal usage, but in an api type, wouldn't a
proper enum be nicer? also would allow differentiating datastore types
more easily in client code (if just for display purposes)
NonRemovable, Mounted, NotMounted
> /// A list of usages of the past (last Month).
> #[serde(skip_serializing_if = "Option::is_none")]
> pub history: Option<Vec<Option<f64>>>,
> @@ -1477,12 +1483,13 @@ pub struct DataStoreStatusListItem {
> }
>
> impl DataStoreStatusListItem {
> - pub fn empty(store: &str, err: Option<String>) -> Self {
> + pub fn empty(store: &str, err: Option<String>, mount_status: Option<bool>) -> Self {
> DataStoreStatusListItem {
> store: store.to_owned(),
> total: None,
> used: None,
> avail: None,
> + mount_status,
> history: None,
> history_start: None,
> history_delta: None,
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index a12262e7..a9d9040f 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1310,8 +1310,8 @@ pub fn get_datastore_list(
>
> let mut list = Vec::new();
>
> - for (store, (_, data)) in &config.sections {
> - let acl_path = &["datastore", store];
> + for (store, (_, data)) in config.sections {
> + let acl_path = &["datastore", &store];
> let user_privs = user_info.lookup_privs(&auth_id, acl_path);
> let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
>
> @@ -1322,15 +1322,21 @@ pub fn get_datastore_list(
> }
> }
>
> + let store_config: DataStoreConfig = serde_json::from_value(data)?;
> +
> + let mount_status = store_config
> + .get_mount_point()
> + .zip(store_config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
another variant of this helper ;)
> +
> if allowed || allow_id {
> list.push(DataStoreListItem {
> store: store.clone(),
> - comment: if !allowed {
> - None
> - } else {
> - data["comment"].as_str().map(String::from)
> - },
> - maintenance: data["maintenance-mode"].as_str().map(String::from),
> + comment: store_config.comment.filter(|_| allowed),
> + mount_status,
> + maintenance: store_config.maintenance_mode,
> });
> }
> }
> diff --git a/src/api2/status/mod.rs b/src/api2/status/mod.rs
> index 113aa985..508331fe 100644
> --- a/src/api2/status/mod.rs
> +++ b/src/api2/status/mod.rs
> @@ -10,11 +10,12 @@ use proxmox_schema::api;
> use proxmox_sortable_macro::sortable;
>
> use pbs_api_types::{
> - Authid, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> + Authid, DataStoreConfig, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT,
> + PRIV_DATASTORE_BACKUP,
> };
>
> use pbs_config::CachedUserInfo;
> -use pbs_datastore::DataStore;
> +use pbs_datastore::{is_datastore_mounted_at, DataStore};
>
> use crate::server::metric_collection::rrd::extract_rrd_data;
> use crate::tools::statistics::linear_regression;
> @@ -51,10 +52,25 @@ pub async fn datastore_status(
> for (store, (_, _)) in &config.sections {
> let user_privs = user_info.lookup_privs(&auth_id, &["datastore", store]);
> let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
> +
> + let store_config = config.lookup::<DataStoreConfig>("datastore", store)?;
> +
> + let mount_status = store_config
> + .get_mount_point()
> + .zip(store_config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> +
> + if let Some(false) = mount_status {
> + list.push(DataStoreStatusListItem::empty(store, None, mount_status));
> + continue;
> + }
> +
> if !allowed {
> if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
> if can_access_any_namespace(datastore, &auth_id, &user_info) {
> - list.push(DataStoreStatusListItem::empty(store, None));
> + list.push(DataStoreStatusListItem::empty(store, None, mount_status));
> }
> }
> continue;
> @@ -63,7 +79,11 @@ pub async fn datastore_status(
> let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
> Ok(datastore) => datastore,
> Err(err) => {
> - list.push(DataStoreStatusListItem::empty(store, Some(err.to_string())));
> + list.push(DataStoreStatusListItem::empty(
> + store,
> + Some(err.to_string()),
> + mount_status,
> + ));
> continue;
> }
> };
> @@ -74,6 +94,7 @@ pub async fn datastore_status(
> total: Some(status.total),
> used: Some(status.used),
> avail: Some(status.available),
> + mount_status,
> history: None,
> history_start: None,
> history_delta: None,
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem
2024-11-21 14:27 ` Fabian Grünbichler
@ 2024-11-21 14:41 ` Hannes Laimer
0 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-21 14:41 UTC (permalink / raw)
To: Proxmox Backup Server development discussion; +Cc: pbs-devel
On Thu Nov 21, 2024 at 3:27 PM CET, Fabian Grünbichler wrote:
> On November 13, 2024 4:00 pm, Hannes Laimer wrote:
>> Only removable datastores have a mount status, so normal ones will have
>> `None`, and for removable ones it is either mounted (`Some(true)`) or
>> not mounted (`Some(false)`).
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> changes since v12:
>> * replace is_availabl+removable field combo, with single mount_status
>> field
>>
>> pbs-api-types/src/datastore.rs | 9 ++++++++-
>> src/api2/admin/datastore.rs | 22 ++++++++++++++--------
>> src/api2/status/mod.rs | 29 +++++++++++++++++++++++++----
>> 3 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index 888f5d5b..e111d692 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -454,6 +454,9 @@ impl DataStoreConfig {
>> pub struct DataStoreListItem {
>> pub store: String,
>> pub comment: Option<String>,
>> + /// Is datastore mounted, None for not-removable datastores
>> + #[serde(skip_serializing_if = "Option::is_none")]
>> + pub mount_status: Option<bool>,
>
> Option<bool> is okay for internal usage, but in an api type, wouldn't a
> proper enum be nicer?
>
> NonRemovable, Mounted, NotMounted
>
I had, but `NonRemovable` kind of bothered me since it is not really a
mount status. But what you are saying makes sense.
>> /// If the datastore is in maintenance mode, information about it
>> #[serde(skip_serializing_if = "Option::is_none")]
>> pub maintenance: Option<String>,
>> @@ -1453,6 +1456,9 @@ pub struct DataStoreStatusListItem {
>> /// The available bytes of the underlying storage. (-1 on error)
>> #[serde(skip_serializing_if = "Option::is_none")]
>> pub avail: Option<u64>,
>> + /// The datastore is mounted, None for not-removable datastores
>> + #[serde(skip_serializing_if = "Option::is_none")]
>> + pub mount_status: Option<bool>,
>
> Option<bool> is okay for internal usage, but in an api type, wouldn't a
> proper enum be nicer? also would allow differentiating datastore types
> more easily in client code (if just for display purposes)
>
> NonRemovable, Mounted, NotMounted
>
>> /// A list of usages of the past (last Month).
>> #[serde(skip_serializing_if = "Option::is_none")]
>> pub history: Option<Vec<Option<f64>>>,
>> @@ -1477,12 +1483,13 @@ pub struct DataStoreStatusListItem {
>> }
>>
>> impl DataStoreStatusListItem {
>> - pub fn empty(store: &str, err: Option<String>) -> Self {
>> + pub fn empty(store: &str, err: Option<String>, mount_status: Option<bool>) -> Self {
>> DataStoreStatusListItem {
>> store: store.to_owned(),
>> total: None,
>> used: None,
>> avail: None,
>> + mount_status,
>> history: None,
>> history_start: None,
>> history_delta: None,
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index a12262e7..a9d9040f 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -1310,8 +1310,8 @@ pub fn get_datastore_list(
>>
>> let mut list = Vec::new();
>>
>> - for (store, (_, data)) in &config.sections {
>> - let acl_path = &["datastore", store];
>> + for (store, (_, data)) in config.sections {
>> + let acl_path = &["datastore", &store];
>> let user_privs = user_info.lookup_privs(&auth_id, acl_path);
>> let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
>>
>> @@ -1322,15 +1322,21 @@ pub fn get_datastore_list(
>> }
>> }
>>
>> + let store_config: DataStoreConfig = serde_json::from_value(data)?;
>> +
>> + let mount_status = store_config
>> + .get_mount_point()
>> + .zip(store_config.backing_device.as_ref())
>> + .map(|(mount_point, device_uuid)| {
>> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
>> + });
>
> another variant of this helper ;)
>
>> +
>> if allowed || allow_id {
>> list.push(DataStoreListItem {
>> store: store.clone(),
>> - comment: if !allowed {
>> - None
>> - } else {
>> - data["comment"].as_str().map(String::from)
>> - },
>> - maintenance: data["maintenance-mode"].as_str().map(String::from),
>> + comment: store_config.comment.filter(|_| allowed),
>> + mount_status,
>> + maintenance: store_config.maintenance_mode,
>> });
>> }
>> }
>> diff --git a/src/api2/status/mod.rs b/src/api2/status/mod.rs
>> index 113aa985..508331fe 100644
>> --- a/src/api2/status/mod.rs
>> +++ b/src/api2/status/mod.rs
>> @@ -10,11 +10,12 @@ use proxmox_schema::api;
>> use proxmox_sortable_macro::sortable;
>>
>> use pbs_api_types::{
>> - Authid, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
>> + Authid, DataStoreConfig, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT,
>> + PRIV_DATASTORE_BACKUP,
>> };
>>
>> use pbs_config::CachedUserInfo;
>> -use pbs_datastore::DataStore;
>> +use pbs_datastore::{is_datastore_mounted_at, DataStore};
>>
>> use crate::server::metric_collection::rrd::extract_rrd_data;
>> use crate::tools::statistics::linear_regression;
>> @@ -51,10 +52,25 @@ pub async fn datastore_status(
>> for (store, (_, _)) in &config.sections {
>> let user_privs = user_info.lookup_privs(&auth_id, &["datastore", store]);
>> let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
>> +
>> + let store_config = config.lookup::<DataStoreConfig>("datastore", store)?;
>> +
>> + let mount_status = store_config
>> + .get_mount_point()
>> + .zip(store_config.backing_device.as_ref())
>> + .map(|(mount_point, device_uuid)| {
>> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
>> + });
>> +
>> + if let Some(false) = mount_status {
>> + list.push(DataStoreStatusListItem::empty(store, None, mount_status));
>> + continue;
>> + }
>> +
>> if !allowed {
>> if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
>> if can_access_any_namespace(datastore, &auth_id, &user_info) {
>> - list.push(DataStoreStatusListItem::empty(store, None));
>> + list.push(DataStoreStatusListItem::empty(store, None, mount_status));
>> }
>> }
>> continue;
>> @@ -63,7 +79,11 @@ pub async fn datastore_status(
>> let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
>> Ok(datastore) => datastore,
>> Err(err) => {
>> - list.push(DataStoreStatusListItem::empty(store, Some(err.to_string())));
>> + list.push(DataStoreStatusListItem::empty(
>> + store,
>> + Some(err.to_string()),
>> + mount_status,
>> + ));
>> continue;
>> }
>> };
>> @@ -74,6 +94,7 @@ pub async fn datastore_status(
>> total: Some(status.total),
>> used: Some(status.used),
>> avail: Some(status.available),
>> + mount_status,
>> history: None,
>> history_start: None,
>> history_delta: None,
>> --
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 10/26] bin: manager: add (un)mount command
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (8 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 11/26] add auto-mounting for removable datastores Hannes Laimer
` (16 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
We can't just directly delegate these commands to the API endpoints
since both mounting and unmounting are done in a worker, and that one
would be killed when the parent ends. In this case that would be the CLI
process, which basically ends right after spwaning the worker.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
pbs-config/src/datastore.rs | 14 ++++
src/bin/proxmox_backup_manager/datastore.rs | 76 ++++++++++++++++++++-
2 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index dc5bb3da..396dcb37 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -62,6 +62,20 @@ pub fn complete_datastore_name(_arg: &str, _param: &HashMap<String, String>) ->
}
}
+pub fn complete_removable_datastore_name(
+ _arg: &str,
+ _param: &HashMap<String, String>,
+) -> Vec<String> {
+ match config() {
+ Ok((data, _digest)) => data
+ .sections
+ .into_iter()
+ .filter_map(|(name, (_, c))| c.get("backing-device").map(|_| name))
+ .collect(),
+ Err(_) => Vec::new(),
+ }
+}
+
pub fn complete_acl_path(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
let mut list = vec![
String::from("/"),
diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 383bcd24..f2795b39 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -1,4 +1,4 @@
-use anyhow::Error;
+use anyhow::{format_err, Error};
use serde_json::Value;
use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
@@ -40,6 +40,34 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
Ok(Value::Null)
}
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ store: {
+ schema: DATASTORE_SCHEMA,
+ },
+ digest: {
+ optional: true,
+ schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
+ },
+ },
+ },
+)]
+/// Mount a removable datastore.
+async fn mount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
+ param["node"] = "localhost".into();
+
+ let info = &api2::admin::datastore::API_METHOD_MOUNT;
+ let result = match info.handler {
+ ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+ _ => unreachable!(),
+ };
+
+ crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+ Ok(())
+}
+
#[api(
input: {
properties: {
@@ -99,6 +127,34 @@ async fn create_datastore(mut param: Value) -> Result<Value, Error> {
Ok(Value::Null)
}
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ store: {
+ schema: DATASTORE_SCHEMA,
+ },
+ digest: {
+ optional: true,
+ schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
+ },
+ },
+ },
+)]
+/// Unmount a removable datastore.
+async fn unmount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
+ param["node"] = "localhost".into();
+
+ let info = &api2::admin::datastore::API_METHOD_UNMOUNT;
+ let result = match info.handler {
+ ApiHandler::Async(handler) => (handler)(param, info, rpcenv).await?,
+ _ => unreachable!(),
+ };
+
+ crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+ Ok(())
+}
+
#[api(
protected: true,
input: {
@@ -142,6 +198,15 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
pub fn datastore_commands() -> CommandLineInterface {
let cmd_def = CliCommandMap::new()
.insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
+ .insert(
+ "mount",
+ CliCommand::new(&API_METHOD_MOUNT_DATASTORE)
+ .arg_param(&["store"])
+ .completion_cb(
+ "store",
+ pbs_config::datastore::complete_removable_datastore_name,
+ ),
+ )
.insert(
"show",
CliCommand::new(&API_METHOD_SHOW_DATASTORE)
@@ -152,6 +217,15 @@ pub fn datastore_commands() -> CommandLineInterface {
"create",
CliCommand::new(&API_METHOD_CREATE_DATASTORE).arg_param(&["name", "path"]),
)
+ .insert(
+ "unmount",
+ CliCommand::new(&API_METHOD_UNMOUNT_DATASTORE)
+ .arg_param(&["store"])
+ .completion_cb(
+ "store",
+ pbs_config::datastore::complete_removable_datastore_name,
+ ),
+ )
.insert(
"update",
CliCommand::new(&api2::config::datastore::API_METHOD_UPDATE_DATASTORE)
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 11/26] add auto-mounting for removable datastores
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (9 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 10/26] bin: manager: add (un)mount command Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:34 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 12/26] datastore: handle deletion of removable datastore properly Hannes Laimer
` (15 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
If a device houses multiple datastore, none of them will be mounted
automatically. If a device only contains a single datastore it will be
mounted automatically. The reason for not mounting multiple datastore
automatically is that we don't know which is actually wanted, and since
mounting all means also all have to be unmounted manually, it made sense
to have the user choose which to mount.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changes since v12:
* make service not dynamic
* don't logs UUIDs that don't contains known datastores
debian/proxmox-backup-server.install | 1 +
debian/proxmox-backup-server.udev | 3 +
etc/Makefile | 3 +-
etc/removable-device-attach@.service | 8 +++
src/bin/proxmox_backup_manager/datastore.rs | 62 ++++++++++++++++++++-
5 files changed, 75 insertions(+), 2 deletions(-)
create mode 100644 etc/removable-device-attach@.service
diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
index 79757ead..ff581e3d 100644
--- a/debian/proxmox-backup-server.install
+++ b/debian/proxmox-backup-server.install
@@ -4,6 +4,7 @@ etc/proxmox-backup-daily-update.service /lib/systemd/system/
etc/proxmox-backup-daily-update.timer /lib/systemd/system/
etc/proxmox-backup-proxy.service /lib/systemd/system/
etc/proxmox-backup.service /lib/systemd/system/
+etc/removable-device-attach@.service /lib/systemd/system/
usr/bin/pmt
usr/bin/pmtx
usr/bin/proxmox-tape
diff --git a/debian/proxmox-backup-server.udev b/debian/proxmox-backup-server.udev
index afdfb2bc..e21b8bc7 100644
--- a/debian/proxmox-backup-server.udev
+++ b/debian/proxmox-backup-server.udev
@@ -16,3 +16,6 @@ SUBSYSTEM=="scsi_generic", SUBSYSTEMS=="scsi", ATTRS{type}=="1", ENV{ID_SCSI_SER
SYMLINK+="tape/by-id/scsi-$env{ID_SCSI_SERIAL}-sg"
LABEL="persistent_storage_tape_end"
+
+# triggers the mounting of a removable device
+ACTION=="add", SUBSYSTEM=="block", ENV{ID_FS_UUID}!="", TAG+="systemd", ENV{SYSTEMD_WANTS}="removable-device-attach@$env{ID_FS_UUID}"
\ No newline at end of file
diff --git a/etc/Makefile b/etc/Makefile
index 42f639f6..b206b9ca 100644
--- a/etc/Makefile
+++ b/etc/Makefile
@@ -2,12 +2,13 @@ include ../defines.mk
UNITS := \
proxmox-backup-daily-update.timer \
+ removable-device-attach@.service \
DYNAMIC_UNITS := \
proxmox-backup-banner.service \
proxmox-backup-daily-update.service \
proxmox-backup.service \
- proxmox-backup-proxy.service
+ proxmox-backup-proxy.service \
all: $(UNITS) $(DYNAMIC_UNITS) pbs-enterprise.list
diff --git a/etc/removable-device-attach@.service b/etc/removable-device-attach@.service
new file mode 100644
index 00000000..e10d1ea3
--- /dev/null
+++ b/etc/removable-device-attach@.service
@@ -0,0 +1,8 @@
+[Unit]
+Description=Try to mount the removable device of a datastore with uuid '%i'.
+After=proxmox-backup-proxy.service
+Requires=proxmox-backup-proxy.service
+
+[Service]
+Type=simple
+ExecStart=/usr/sbin/proxmox-backup-manager datastore uuid-mount %i
diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index f2795b39..05f35279 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -1,4 +1,4 @@
-use anyhow::{format_err, Error};
+use anyhow::{bail, format_err, Error};
use serde_json::Value;
use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
@@ -195,6 +195,62 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
Ok(())
}
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ uuid: {
+ type: String,
+ description: "The UUID of the device that should be mounted",
+ },
+ "output-format": {
+ schema: OUTPUT_FORMAT,
+ optional: true,
+ },
+ },
+ },
+)]
+/// Try mounting a removable datastore given the UUID.
+async fn uuid_mount(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let uuid = param["uuid"]
+ .as_str()
+ .ok_or_else(|| format_err!("uuid has to be specified"))?;
+
+ let info = &api2::config::datastore::API_METHOD_LIST_DATASTORES;
+ let data: Value = match info.handler {
+ ApiHandler::Sync(handler) => (handler)(serde_json::json!({}), info, rpcenv)?,
+ _ => unreachable!(),
+ };
+
+ let matching_stores = data.as_array().map_or(Vec::new(), |list| {
+ list.iter()
+ .filter_map(Value::as_object)
+ .filter(|store| store.get("backing-device").map_or(false, |d| d.eq(&uuid)))
+ .collect()
+ });
+
+ if matching_stores.len() != 1 {
+ return Ok(Value::Null);
+ }
+
+ let store_name = matching_stores
+ .get(0)
+ .and_then(|s| s.get("name").and_then(Value::as_str));
+ if let Some(store_name) = store_name {
+ let info = &api2::admin::datastore::API_METHOD_MOUNT;
+ let mount_param = serde_json::json!({
+ "store": store_name,
+ });
+ let result = match info.handler {
+ ApiHandler::Sync(handler) => (handler)(mount_param, info, rpcenv)?,
+ _ => unreachable!(),
+ };
+ crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+ return Ok(Value::Null);
+ }
+ bail!("'{uuid}' is not associated with any datastore")
+}
+
pub fn datastore_commands() -> CommandLineInterface {
let cmd_def = CliCommandMap::new()
.insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
@@ -240,6 +296,10 @@ pub fn datastore_commands() -> CommandLineInterface {
pbs_config::datastore::complete_calendar_event,
),
)
+ .insert(
+ "uuid-mount",
+ CliCommand::new(&API_METHOD_UUID_MOUNT).arg_param(&["uuid"]),
+ )
.insert(
"remove",
CliCommand::new(&API_METHOD_DELETE_DATASTORE)
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 11/26] add auto-mounting for removable datastores
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 11/26] add auto-mounting for removable datastores Hannes Laimer
@ 2024-11-21 14:34 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:34 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> If a device houses multiple datastore, none of them will be mounted
> automatically. If a device only contains a single datastore it will be
> mounted automatically. The reason for not mounting multiple datastore
> automatically is that we don't know which is actually wanted, and since
> mounting all means also all have to be unmounted manually, it made sense
> to have the user choose which to mount.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
did you reject the variant with a custom helper binary instead of an
internal command? ;)
> ---
> changes since v12:
> * make service not dynamic
> * don't logs UUIDs that don't contains known datastores
>
> debian/proxmox-backup-server.install | 1 +
> debian/proxmox-backup-server.udev | 3 +
> etc/Makefile | 3 +-
> etc/removable-device-attach@.service | 8 +++
> src/bin/proxmox_backup_manager/datastore.rs | 62 ++++++++++++++++++++-
> 5 files changed, 75 insertions(+), 2 deletions(-)
> create mode 100644 etc/removable-device-attach@.service
>
> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
> index 79757ead..ff581e3d 100644
> --- a/debian/proxmox-backup-server.install
> +++ b/debian/proxmox-backup-server.install
> @@ -4,6 +4,7 @@ etc/proxmox-backup-daily-update.service /lib/systemd/system/
> etc/proxmox-backup-daily-update.timer /lib/systemd/system/
> etc/proxmox-backup-proxy.service /lib/systemd/system/
> etc/proxmox-backup.service /lib/systemd/system/
> +etc/removable-device-attach@.service /lib/systemd/system/
> usr/bin/pmt
> usr/bin/pmtx
> usr/bin/proxmox-tape
> diff --git a/debian/proxmox-backup-server.udev b/debian/proxmox-backup-server.udev
> index afdfb2bc..e21b8bc7 100644
> --- a/debian/proxmox-backup-server.udev
> +++ b/debian/proxmox-backup-server.udev
> @@ -16,3 +16,6 @@ SUBSYSTEM=="scsi_generic", SUBSYSTEMS=="scsi", ATTRS{type}=="1", ENV{ID_SCSI_SER
> SYMLINK+="tape/by-id/scsi-$env{ID_SCSI_SERIAL}-sg"
>
> LABEL="persistent_storage_tape_end"
> +
> +# triggers the mounting of a removable device
> +ACTION=="add", SUBSYSTEM=="block", ENV{ID_FS_UUID}!="", TAG+="systemd", ENV{SYSTEMD_WANTS}="removable-device-attach@$env{ID_FS_UUID}"
> \ No newline at end of file
> diff --git a/etc/Makefile b/etc/Makefile
> index 42f639f6..b206b9ca 100644
> --- a/etc/Makefile
> +++ b/etc/Makefile
> @@ -2,12 +2,13 @@ include ../defines.mk
>
> UNITS := \
> proxmox-backup-daily-update.timer \
> + removable-device-attach@.service \
nit: the last line shouldn't have a trailing \
>
> DYNAMIC_UNITS := \
> proxmox-backup-banner.service \
> proxmox-backup-daily-update.service \
> proxmox-backup.service \
> - proxmox-backup-proxy.service
> + proxmox-backup-proxy.service \
same here
>
> all: $(UNITS) $(DYNAMIC_UNITS) pbs-enterprise.list
>
> diff --git a/etc/removable-device-attach@.service b/etc/removable-device-attach@.service
> new file mode 100644
> index 00000000..e10d1ea3
> --- /dev/null
> +++ b/etc/removable-device-attach@.service
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Try to mount the removable device of a datastore with uuid '%i'.
> +After=proxmox-backup-proxy.service
> +Requires=proxmox-backup-proxy.service
> +
> +[Service]
> +Type=simple
> +ExecStart=/usr/sbin/proxmox-backup-manager datastore uuid-mount %i
> diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
> index f2795b39..05f35279 100644
> --- a/src/bin/proxmox_backup_manager/datastore.rs
> +++ b/src/bin/proxmox_backup_manager/datastore.rs
> @@ -1,4 +1,4 @@
> -use anyhow::{format_err, Error};
> +use anyhow::{bail, format_err, Error};
> use serde_json::Value;
>
> use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
> @@ -195,6 +195,62 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
> Ok(())
> }
>
> +#[api(
> + protected: true,
> + input: {
> + properties: {
> + uuid: {
> + type: String,
> + description: "The UUID of the device that should be mounted",
> + },
> + "output-format": {
> + schema: OUTPUT_FORMAT,
> + optional: true,
> + },
> + },
> + },
> +)]
> +/// Try mounting a removable datastore given the UUID.
> +async fn uuid_mount(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> + let uuid = param["uuid"]
> + .as_str()
> + .ok_or_else(|| format_err!("uuid has to be specified"))?;
> +
> + let info = &api2::config::datastore::API_METHOD_LIST_DATASTORES;
> + let data: Value = match info.handler {
> + ApiHandler::Sync(handler) => (handler)(serde_json::json!({}), info, rpcenv)?,
> + _ => unreachable!(),
> + };
couldn't this just load the datastore.cfg ?
> +
> + let matching_stores = data.as_array().map_or(Vec::new(), |list| {
> + list.iter()
> + .filter_map(Value::as_object)
> + .filter(|store| store.get("backing-device").map_or(false, |d| d.eq(&uuid)))
> + .collect()
> + });
then this could use regular methods ;)
> +
> + if matching_stores.len() != 1 {
> + return Ok(Value::Null);
> + }
nit: see below..
> +
> + let store_name = matching_stores
> + .get(0)
> + .and_then(|s| s.get("name").and_then(Value::as_str));
> + if let Some(store_name) = store_name {
> + let info = &api2::admin::datastore::API_METHOD_MOUNT;
> + let mount_param = serde_json::json!({
> + "store": store_name,
> + });
> + let result = match info.handler {
> + ApiHandler::Sync(handler) => (handler)(mount_param, info, rpcenv)?,
> + _ => unreachable!(),
> + };
> + crate::wait_for_local_worker(result.as_str().unwrap()).await?;
this could use the mount wrapper we already have here in manager..
> + return Ok(Value::Null);
> + }
> + bail!("'{uuid}' is not associated with any datastore")
nit: this bail is dead code?
I'd just restructure it, easier to read..
// only auto-mount if 1:1 mapping between datastore and device
if len == 1 {
mount it
}
return Ok(Value::Null))
> +}
> +
> pub fn datastore_commands() -> CommandLineInterface {
> let cmd_def = CliCommandMap::new()
> .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
> @@ -240,6 +296,10 @@ pub fn datastore_commands() -> CommandLineInterface {
> pbs_config::datastore::complete_calendar_event,
> ),
> )
> + .insert(
> + "uuid-mount",
> + CliCommand::new(&API_METHOD_UUID_MOUNT).arg_param(&["uuid"]),
> + )
> .insert(
> "remove",
> CliCommand::new(&API_METHOD_DELETE_DATASTORE)
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 12/26] datastore: handle deletion of removable datastore properly
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (10 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 11/26] add auto-mounting for removable datastores Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:39 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 13/26] docs: add removable datastores section Hannes Laimer
` (14 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Data deletion is only possible if the datastore is mounted, won't attempt
mounting it for the purpose of deleting data is made.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
pbs-datastore/src/datastore.rs | 4 +++-
src/api2/config/datastore.rs | 37 +++++++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index cadf9245..83e4dcb0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1525,7 +1525,9 @@ impl DataStore {
// weird, but ok
}
Err(err) if err.is_errno(nix::errno::Errno::EBUSY) => {
- warn!("Cannot delete datastore directory (is it a mount point?).")
+ if datastore_config.backing_device.is_none() {
+ warn!("Cannot delete datastore directory (is it a mount point?).")
+ }
}
Err(err) if err.is_errno(nix::errno::Errno::ENOTEMPTY) => {
warn!("Datastore directory not empty, not deleting.")
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 9140a7a4..60bff9e2 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -1,4 +1,4 @@
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
use ::serde::{Deserialize, Serialize};
use anyhow::{bail, Error};
@@ -29,6 +29,7 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
use crate::api2::config::verify::delete_verification_job;
use pbs_config::CachedUserInfo;
+use pbs_datastore::is_datastore_mounted_at;
use proxmox_rest_server::WorkerTask;
use crate::server::jobstate;
@@ -557,6 +558,21 @@ pub async fn delete_datastore(
http_bail!(NOT_FOUND, "datastore '{}' does not exist.", name);
}
+ let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
+ let mount_status = store_config
+ .get_mount_point()
+ .zip(store_config.backing_device.as_ref())
+ .map(|(mount_point, device_uuid)| {
+ is_datastore_mounted_at(mount_point, device_uuid.to_string())
+ });
+
+ if destroy_data && mount_status == Some(false) {
+ http_bail!(
+ BAD_REQUEST,
+ "cannot destroy data on '{name}' unless the datastore is mounted"
+ );
+ }
+
if !keep_job_configs {
for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
delete_verification_job(job.config.id, None, rpcenv)?
@@ -583,6 +599,19 @@ pub async fn delete_datastore(
let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+ let name_copy = name.clone();
+ if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
+ {
+ let sock = proxmox_daemon::command_socket::path_from_pid(proxy_pid);
+ let _ = proxmox_daemon::command_socket::send_raw(
+ sock,
+ &format!(
+ "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
+ name_copy
+ ),
+ )
+ .await;
+ };
let upid = WorkerTask::new_thread(
"delete-datastore",
@@ -595,6 +624,12 @@ pub async fn delete_datastore(
// ignore errors
let _ = jobstate::remove_state_file("prune", &name);
let _ = jobstate::remove_state_file("garbage_collection", &name);
+ if destroy_data {
+ if let Some(mount_point) = store_config.get_mount_point() {
+ let _ = unmount_by_mountpoint(Path::new(&mount_point));
+ let _ = std::fs::remove_dir(&mount_point);
+ }
+ }
if let Err(err) =
proxmox_async::runtime::block_on(crate::server::notify_datastore_removed())
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 12/26] datastore: handle deletion of removable datastore properly
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 12/26] datastore: handle deletion of removable datastore properly Hannes Laimer
@ 2024-11-21 14:39 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:39 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Data deletion is only possible if the datastore is mounted, won't attempt
> mounting it for the purpose of deleting data is made.
this commit message is missing some word (or has a few too many?)
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> pbs-datastore/src/datastore.rs | 4 +++-
> src/api2/config/datastore.rs | 37 +++++++++++++++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index cadf9245..83e4dcb0 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1525,7 +1525,9 @@ impl DataStore {
> // weird, but ok
> }
> Err(err) if err.is_errno(nix::errno::Errno::EBUSY) => {
> - warn!("Cannot delete datastore directory (is it a mount point?).")
> + if datastore_config.backing_device.is_none() {
> + warn!("Cannot delete datastore directory (is it a mount point?).")
> + }
> }
> Err(err) if err.is_errno(nix::errno::Errno::ENOTEMPTY) => {
> warn!("Datastore directory not empty, not deleting.")
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 9140a7a4..60bff9e2 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -1,4 +1,4 @@
> -use std::path::PathBuf;
> +use std::path::{Path, PathBuf};
>
> use ::serde::{Deserialize, Serialize};
> use anyhow::{bail, Error};
> @@ -29,6 +29,7 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
> use crate::api2::config::verify::delete_verification_job;
> use pbs_config::CachedUserInfo;
>
> +use pbs_datastore::is_datastore_mounted_at;
> use proxmox_rest_server::WorkerTask;
>
> use crate::server::jobstate;
> @@ -557,6 +558,21 @@ pub async fn delete_datastore(
> http_bail!(NOT_FOUND, "datastore '{}' does not exist.", name);
> }
>
> + let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
> + let mount_status = store_config
> + .get_mount_point()
> + .zip(store_config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
another instance of this ;)
> +
> + if destroy_data && mount_status == Some(false) {
> + http_bail!(
> + BAD_REQUEST,
> + "cannot destroy data on '{name}' unless the datastore is mounted"
> + );
> + }
> +
> if !keep_job_configs {
> for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
> delete_verification_job(job.config.id, None, rpcenv)?
> @@ -583,6 +599,19 @@ pub async fn delete_datastore(
>
> let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> + let name_copy = name.clone();
nit: why/leftover?
> + if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> + {
> + let sock = proxmox_daemon::command_socket::path_from_pid(proxy_pid);
> + let _ = proxmox_daemon::command_socket::send_raw(
> + sock,
> + &format!(
> + "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
> + name_copy
> + ),
> + )
> + .await;
> + };
>
> let upid = WorkerTask::new_thread(
> "delete-datastore",
> @@ -595,6 +624,12 @@ pub async fn delete_datastore(
> // ignore errors
> let _ = jobstate::remove_state_file("prune", &name);
> let _ = jobstate::remove_state_file("garbage_collection", &name);
> + if destroy_data {
> + if let Some(mount_point) = store_config.get_mount_point() {
> + let _ = unmount_by_mountpoint(Path::new(&mount_point));
> + let _ = std::fs::remove_dir(&mount_point);
errors here should be logged I think? ignoring them is okay (IMHO the
same applies above for the state files..)
> + }
> + }
>
> if let Err(err) =
> proxmox_async::runtime::block_on(crate::server::notify_datastore_removed())
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 13/26] docs: add removable datastores section
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (11 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 12/26] datastore: handle deletion of removable datastore properly Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-18 10:43 ` Maximiliano Sandoval
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 14/26] ui: add partition selector form Hannes Laimer
` (13 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
docs/storage.rst | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/docs/storage.rst b/docs/storage.rst
index f1e15d52..d9871e79 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -165,6 +165,44 @@ following command creates a new datastore called ``store1`` on
# proxmox-backup-manager datastore create store1 /backup/disk1/store1
+Removable Datastores
+^^^^^^^^^^^^^^^^^^^^
+Removable datastores have a ``backing-device`` associated with them, they can be
+mounted and unmounted. Other than that they behave the same way a normal datastore
+would.
+
+They can be created on already correctly formatted partitions, which, as with normal
+datastores, should be either ``ext4`` or ``xfs``. It is also possible to create them
+on completely unused disks through "Administartion" > "Disks / Storage" > "Directory",
+using this method the disk will be partitioned and formatted automatically for the datastore.
+
+Devices with only one datastore on them will be mounted automatically. It is possible to create a
+removable datastore on one PBS and use it on multiple instances, the device just has to be added
+on each instance as a removable datastore by checking "reuse datastore" on creation.
+If the device already contains a datastore at the specified path it'll just be added as
+a new datastore to the PBS instance and will be mounted whenever plugged in. Unmounting has
+to be done through the UI by clicking "Unmount" on the summary page or using the CLI.
+
+A single device can house multiple datastores, they only limitation is that they are not
+allowed to be nested.
+
+.. code-block:: console
+
+ # proxmox-backup-manager datastore unmount store1
+
+both will wait for any running tasks to finish and unmount the device.
+
+All removable datastores are mounted under /mnt/datastore/<name>, and the specified path
+refers to the path on the device.
+
+All datastores present on a device can be listed using ``proxmox-backup-debug``.
+
+.. code-block:: console
+
+ # proxmox-backup-debug inspect device /dev/...
+
+
+
Managing Datastores
^^^^^^^^^^^^^^^^^^^
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 14/26] ui: add partition selector form
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (12 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 13/26] docs: add removable datastores section Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 15/26] ui: add removable datastore creation support Hannes Laimer
` (12 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/Makefile | 1 +
www/form/PartitionSelector.js | 81 +++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+)
create mode 100644 www/form/PartitionSelector.js
diff --git a/www/Makefile b/www/Makefile
index 609a0ba6..45adfc54 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -49,6 +49,7 @@ JSSRC= \
form/NamespaceMaxDepth.js \
form/CalendarEvent.js \
form/PermissionPathSelector.js \
+ form/PartitionSelector.js \
form/GroupSelector.js \
form/GroupFilter.js \
form/VerifyOutdatedAfter.js \
diff --git a/www/form/PartitionSelector.js b/www/form/PartitionSelector.js
new file mode 100644
index 00000000..162dbe41
--- /dev/null
+++ b/www/form/PartitionSelector.js
@@ -0,0 +1,81 @@
+Ext.define('pbs-partition-list', {
+ extend: 'Ext.data.Model',
+ fields: ['name', 'uuid', 'filesystem', 'devpath', 'size', 'model'],
+ proxy: {
+ type: 'proxmox',
+ url: "/api2/json/nodes/localhost/disks/list?skipsmart=1&include-partitions=1",
+ reader: {
+ transform: (rawData) => rawData.data
+ .flatMap(disk => (disk.partitions
+ .map(part => ({ ...part, model: disk.model })) ?? [])
+ .filter(partition => partition.used === 'filesystem')),
+ },
+ },
+ idProperty: 'devpath',
+
+});
+
+Ext.define('PBS.form.PartitionSelector', {
+ extend: 'Proxmox.form.ComboGrid',
+ alias: 'widget.pbsPartitionSelector',
+
+ allowBlank: false,
+ autoSelect: false,
+ submitEmpty: false,
+ valueField: 'uuid',
+ displayField: 'devpath',
+
+ store: {
+ model: 'pbs-partition-list',
+ autoLoad: true,
+ sorters: 'devpath',
+ },
+ getSubmitData: function() {
+ let me = this;
+ let data = null;
+ if (!me.disabled && me.submitValue && !me.isFileUpload()) {
+ let val = me.getSubmitValue();
+ if (val !== undefined && val !== null && val !== '') {
+ data = {};
+ data[me.getName()] = val;
+ } else if (me.getDeleteEmpty()) {
+ data = {};
+ data.delete = me.getName();
+ }
+ }
+ return data;
+ },
+ listConfig: {
+ columns: [
+ {
+ header: gettext('Path'),
+ sortable: true,
+ dataIndex: 'devpath',
+ renderer: (v, metaData, rec) => Ext.String.htmlEncode(v),
+ flex: 1,
+ },
+ {
+ header: gettext('Filesystem'),
+ sortable: true,
+ dataIndex: 'filesystem',
+ flex: 1,
+ },
+ {
+ header: gettext('Size'),
+ sortable: true,
+ dataIndex: 'size',
+ renderer: Proxmox.Utils.format_size,
+ flex: 1,
+ },
+ {
+ header: gettext('Model'),
+ sortable: true,
+ dataIndex: 'model',
+ flex: 1,
+ },
+ ],
+ viewConfig: {
+ emptyText: 'No usable partitions present',
+ },
+ },
+});
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 15/26] ui: add removable datastore creation support
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (13 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 14/26] ui: add partition selector form Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 16/26] ui: add (un)mount button to summary Hannes Laimer
` (11 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/window/DataStoreEdit.js | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index b8e866df..7b6aff1e 100644
--- a/www/window/DataStoreEdit.js
+++ b/www/window/DataStoreEdit.js
@@ -63,6 +63,20 @@ Ext.define('PBS.DataStoreEdit', {
emptyText: gettext('An absolute path'),
validator: val => val?.trim() !== '/',
},
+ {
+ xtype: 'pmxDisplayEditField',
+ fieldLabel: gettext('Device'),
+ name: 'backing-device',
+ disabled: true,
+ cbind: {
+ editable: '{isCreate}',
+ },
+ editConfig: {
+ xtype: 'pbsPartitionSelector',
+ allowBlank: true,
+ },
+ emptyText: gettext('Device path'),
+ },
],
column2: [
{
@@ -88,6 +102,29 @@ Ext.define('PBS.DataStoreEdit', {
},
],
columnB: [
+ {
+ xtype: 'checkbox',
+ boxLabel: gettext('Removable datastore'),
+ submitValue: false,
+ listeners: {
+ change: function(checkbox, isRemovable) {
+ let inputPanel = checkbox.up('inputpanel');
+ let pathField = inputPanel.down('[name=path]');
+ let uuidField = inputPanel.down('pbsPartitionSelector[name=backing-device]');
+ let uuidEditField = inputPanel.down('[name=backing-device]');
+
+ uuidField.allowBlank = !isRemovable;
+ uuidEditField.setDisabled(!isRemovable);
+ uuidField.setDisabled(!isRemovable);
+ uuidField.setValue('');
+ if (isRemovable) {
+ pathField.setFieldLabel(gettext('On device path'));
+ } else {
+ pathField.setFieldLabel(gettext('Backing Path'));
+ }
+ },
+ },
+ },
{
xtype: 'textfield',
name: 'comment',
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 16/26] ui: add (un)mount button to summary
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (14 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 15/26] ui: add removable datastore creation support Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 17/26] ui: tree: render unmounted datastores correctly Hannes Laimer
` (10 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
And only try to load datastore information if the datastore is
available.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/datastore/Summary.js | 92 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 90 insertions(+), 2 deletions(-)
diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index a932b4e0..49aa3b3c 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -309,7 +309,82 @@ Ext.define('PBS.DataStoreSummary', {
model: 'pve-rrd-datastore',
});
- me.callParent();
+ me.statusStore = Ext.create('Proxmox.data.ObjectStore', {
+ url: `/api2/json/admin/datastore/${me.datastore}/status`,
+ interval: 1000,
+ });
+
+ let unmountBtn = Ext.create('Ext.Button', {
+ text: gettext('Unmount'),
+ hidden: true,
+ handler: () => {
+ Proxmox.Utils.API2Request({
+ url: `/admin/datastore/${me.datastore}/unmount`,
+ method: 'POST',
+ failure: function(response) {
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+ },
+ success: function(response, options) {
+ Ext.create('Proxmox.window.TaskViewer', {
+ upid: response.result.data,
+ }).show();
+ },
+ });
+ },
+ });
+
+ let mountBtn = Ext.create('Ext.Button', {
+ text: gettext('Mount'),
+ hidden: true,
+ handler: () => {
+ Proxmox.Utils.API2Request({
+ url: `/admin/datastore/${me.datastore}/mount`,
+ method: 'POST',
+ failure: function(response) {
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+ },
+ success: function(response, options) {
+ Ext.create('Proxmox.window.TaskViewer', {
+ upid: response.result.data,
+ }).show();
+ },
+ });
+ },
+ });
+
+ Ext.apply(me, {
+ tbar: [unmountBtn, mountBtn, '->', { xtype: 'proxmoxRRDTypeSelector' }],
+ });
+
+ me.mon(me.statusStore, 'load', (s, records, success) => {
+ if (!success) {
+ me.down('pbsDataStoreInfo').fireEvent('deactivate');
+ Proxmox.Utils.API2Request({
+ url: `/config/datastore/${me.datastore}`,
+ success: response => {
+ let mode = response.result.data['maintenance-mode'];
+ let [type, _message] = PBS.Utils.parseMaintenanceMode(mode);
+ if (!response.result.data['backing-device']) {
+ return;
+ }
+ if (!type || type === 'read-only') {
+ unmountBtn.setDisabled(true);
+ mountBtn.setDisabled(false);
+ } else if (type === 'unmount') {
+ unmountBtn.setDisabled(true);
+ mountBtn.setDisabled(true);
+ } else {
+ unmountBtn.setDisabled(false);
+ mountBtn.setDisabled(false);
+ }
+ },
+ });
+ } else {
+ me.down('pbsDataStoreInfo').fireEvent('activate');
+ unmountBtn.setDisabled(false);
+ mountBtn.setDisabled(true);
+ }
+ });
let sp = Ext.state.Manager.getProvider();
me.mon(sp, 'statechange', function(provider, key, value) {
@@ -322,11 +397,17 @@ Ext.define('PBS.DataStoreSummary', {
Proxmox.Utils.updateColumns(me);
});
+ me.callParent();
+
Proxmox.Utils.API2Request({
url: `/config/datastore/${me.datastore}`,
waitMsgTarget: me.down('pbsDataStoreInfo'),
success: function(response) {
- let path = Ext.htmlEncode(response.result.data.path);
+ let data = response.result.data;
+ let path = Ext.htmlEncode(data.path);
+ const removable = Object.prototype.hasOwnProperty.call(data, "backing-device");
+ unmountBtn.setHidden(!removable);
+ mountBtn.setHidden(!removable);
me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
},
@@ -344,6 +425,13 @@ Ext.define('PBS.DataStoreSummary', {
let hasIoTicks = records?.some((rec) => rec?.data?.io_ticks !== undefined);
me.down('#ioDelayChart').setVisible(!success || hasIoTicks);
}, undefined, { single: true });
+ me.on('afterrender', () => {
+ me.statusStore.startUpdate();
+ });
+
+ me.on('destroy', () => {
+ me.statusStore.stopUpdate();
+ });
me.query('proxmoxRRDChart').forEach((chart) => {
chart.setStore(me.rrdstore);
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 17/26] ui: tree: render unmounted datastores correctly
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (15 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 16/26] ui: add (un)mount button to summary Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 18/26] ui: utils: make parseMaintenanceMode more robust Hannes Laimer
` (9 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/NavigationTree.js | 17 ++++++++++++++---
www/css/ext6-pbs.css | 8 ++++++++
www/datastore/DataStoreListSummary.js | 1 +
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/www/NavigationTree.js b/www/NavigationTree.js
index 53c8daff..9c7c9208 100644
--- a/www/NavigationTree.js
+++ b/www/NavigationTree.js
@@ -267,13 +267,24 @@ Ext.define('PBS.view.main.NavigationTree', {
j++;
}
- let [qtip, iconCls] = ['', 'fa fa-database'];
+ let mainIcon = `fa fa-${records[i].data.removable ? 'plug' : 'database'}`;
+ let [qtip, iconCls] = ['', mainIcon];
const maintenance = records[i].data.maintenance;
+
+ // mount-status does only exist for removable datastores
+ const removable_not_mounted = records[i].data['mount-status'] === false;
+ if (removable_not_mounted) {
+ iconCls = `${mainIcon} pmx-tree-icon-custom unplugged`;
+ qtip = gettext('Removable datastore not mounted');
+ }
if (maintenance) {
const [type, message] = PBS.Utils.parseMaintenanceMode(maintenance);
qtip = `${type}${message ? ': ' + message : ''}`;
- let maintenanceTypeCls = type === 'delete' ? 'destroying' : 'maintenance';
- iconCls = `fa fa-database pmx-tree-icon-custom ${maintenanceTypeCls}`;
+ let maintenanceTypeCls = 'maintenance';
+ if (type === 'delete') {
+ maintenanceTypeCls = 'destroying';
+ }
+ iconCls = `${mainIcon} pmx-tree-icon-custom ${maintenanceTypeCls}`;
}
if (getChildTextAt(j).localeCompare(name) !== 0) {
diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index c33ce684..706e681e 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -271,6 +271,10 @@ span.snapshot-comment-column {
content: "\ ";
}
+.x-treelist-item-icon.fa-plug, .pmx-tree-icon-custom.fa-plug {
+ font-size: 12px;
+}
+
/* datastore maintenance */
.pmx-tree-icon-custom.maintenance:after {
content: "\f0ad";
@@ -290,6 +294,10 @@ span.snapshot-comment-column {
color: #888;
}
+.pmx-tree-icon-custom.unplugged:before {
+ color: #888;
+}
+
/*' PBS specific icons */
.pbs-icon-tape {
diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index b908034d..f7ea83e7 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -22,6 +22,7 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
stillbad: 0,
deduplication: 1.0,
error: "",
+ removable: false,
maintenance: '',
},
},
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 18/26] ui: utils: make parseMaintenanceMode more robust
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (16 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 17/26] ui: tree: render unmounted datastores correctly Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 19/26] ui: add datastore status mask for unmounted removable datastores Hannes Laimer
` (8 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/Utils.js | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/www/Utils.js b/www/Utils.js
index 4853be36..7756e9b5 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -740,14 +740,29 @@ Ext.define('PBS.Utils', {
return `${icon} ${value}`;
},
- // FIXME: this "parser" is brittle and relies on the order the arguments will appear in
+ /**
+ * Parses maintenance mode property string.
+ * Examples:
+ * "offline,message=foo" -> ["offline", "foo"]
+ * "offline" -> ["offline", null]
+ * "message=foo,offline" -> ["offline", "foo"]
+ * null/undefined -> [null, null]
+ *
+ * @param {string|null} mode - Maintenance mode string to parse.
+ * @return {Array<string|null>} - Parsed maintenance mode values.
+ */
parseMaintenanceMode: function(mode) {
- let [type, message] = mode.split(/,(.+)/);
- type = type.split("=").pop();
- message = message ? message.split("=")[1]
- .replace(/^"(.*)"$/, '$1')
- .replaceAll('\\"', '"') : null;
- return [type, message];
+ if (!mode) {
+ return [null, null];
+ }
+ return mode.split(',').reduce(([m, msg], pair) => {
+ const [key, value] = pair.split('=');
+ if (key === 'message') {
+ return [m, value.replace(/^"(.*)"$/, '$1').replace(/\\"/g, '"')];
+ } else {
+ return [value ?? key, msg];
+ }
+ }, [null, null]);
},
renderMaintenance: function(mode, activeTasks) {
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 19/26] ui: add datastore status mask for unmounted removable datastores
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (17 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 18/26] ui: utils: make parseMaintenanceMode more robust Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 20/26] ui: maintenance: fix disable msg field if no type is selected Hannes Laimer
` (7 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/css/ext6-pbs.css | 12 ++++++++++++
www/datastore/Summary.js | 21 +++++++++++++--------
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index 706e681e..891189ae 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -261,6 +261,18 @@ span.snapshot-comment-column {
content: "\f0ad";
}
+.pbs-unplugged-mask div.x-mask-msg-text {
+ background: None;
+ padding: 12px 0 0;
+}
+
+.pbs-unplugged-mask:before {
+ font-size: 3em;
+ display: flex;
+ justify-content: center;
+ content: "\f1e6";
+}
+
/* the small icons TODO move to proxmox-widget-toolkit */
.pmx-tree-icon-custom:after {
position: relative;
diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index 49aa3b3c..33b4e18a 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -61,16 +61,21 @@ Ext.define('PBS.DataStoreInfo', {
Proxmox.Utils.API2Request({
url: `/config/datastore/${me.view.datastore}`,
success: function(response) {
- const config = response.result.data;
- if (config['maintenance-mode']) {
- const [_type, msg] = PBS.Utils.parseMaintenanceMode(config['maintenance-mode']);
- me.view.el.mask(
- `${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`,
- 'fa pbs-maintenance-mask',
- );
- } else {
+ let maintenanceString = response.result.data['maintenance-mode'];
+ let removable = !!response.result.data['backing-device'];
+ if (!maintenanceString && !removable) {
me.view.el.mask(gettext('Datastore is not available'));
+ return;
}
+
+ let [_type, msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
+ let isUnplugged = !maintenanceString && removable;
+ let maskMessage = isUnplugged
+ ? gettext('Datastore is not mounted')
+ : `${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`;
+
+ let maskIcon = isUnplugged ? 'fa pbs-unplugged-mask' : 'fa pbs-maintenance-mask';
+ me.view.el.mask(maskMessage, maskIcon);
},
});
return;
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 20/26] ui: maintenance: fix disable msg field if no type is selected
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (18 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 19/26] ui: add datastore status mask for unmounted removable datastores Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 21/26] ui: render 'unmount' maintenance mode correctly Hannes Laimer
` (6 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/window/MaintenanceOptions.js | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 1ee92542..527c3698 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -56,12 +56,17 @@ Ext.define('PBS.window.MaintenanceOptions', {
fieldLabel: gettext('Maintenance Type'),
value: '__default__',
deleteEmpty: true,
+ listeners: {
+ change: (field, newValue) => {
+ Ext.getCmp('message-field').setDisabled(newValue === '__default__');
+ },
+ },
},
{
xtype: 'proxmoxtextfield',
+ id: 'message-field',
name: 'maintenance-msg',
fieldLabel: gettext('Description'),
- // FIXME: disable if maintenance type is none
},
],
},
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 21/26] ui: render 'unmount' maintenance mode correctly
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (19 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 20/26] ui: maintenance: fix disable msg field if no type is selected Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 22/26] api: node: allow creation of removable datastore through directory endpoint Hannes Laimer
` (5 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/Utils.js | 4 +++-
www/window/MaintenanceOptions.js | 10 ++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/www/Utils.js b/www/Utils.js
index 7756e9b5..6bae9b70 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -775,7 +775,7 @@ Ext.define('PBS.Utils', {
let extra = '';
if (activeTasks !== undefined) {
- const conflictingTasks = activeTasks.write + (type === 'offline' ? activeTasks.read : 0);
+ const conflictingTasks = activeTasks.write + (type === 'offline' || type === 'unmount' ? activeTasks.read : 0);
if (conflictingTasks > 0) {
extra += '| <i class="fa fa-spinner fa-pulse fa-fw"></i> ';
@@ -795,6 +795,8 @@ Ext.define('PBS.Utils', {
break;
case 'offline': modeText = gettext("Offline");
break;
+ case 'unmount': modeText = gettext("Unmounting");
+ break;
}
return `${modeText} ${extra}`;
},
diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 527c3698..d7348cb4 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -52,6 +52,7 @@ Ext.define('PBS.window.MaintenanceOptions', {
items: [
{
xtype: 'pbsMaintenanceType',
+ id: 'type-field',
name: 'maintenance-type',
fieldLabel: gettext('Maintenance Type'),
value: '__default__',
@@ -85,6 +86,15 @@ Ext.define('PBS.window.MaintenanceOptions', {
};
}
+ let unmounting = options['maintenance-type'] === 'unmount';
+ let defaultType = options['maintenance-type'] === '__default__';
+ if (unmounting) {
+ options['maintenance-type'] = '';
+ }
+
me.callParent([options]);
+
+ Ext.ComponentManager.get('type-field').setDisabled(unmounting);
+ Ext.ComponentManager.get('message-field').setDisabled(unmounting || defaultType);
},
});
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 22/26] api: node: allow creation of removable datastore through directory endpoint
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (20 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 21/26] ui: render 'unmount' maintenance mode correctly Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:51 ` Fabian Grünbichler
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 23/26] api: node: include removable datastores in directory list Hannes Laimer
` (4 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/node/disks/directory.rs | 59 +++++++++++++++++++++++++++++---
1 file changed, 54 insertions(+), 5 deletions(-)
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 7f540220..7e020e27 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -123,6 +123,11 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
description: "Configure a datastore using the directory.",
type: bool,
optional: true,
+ default: false,
+ },
+ "removable-datastore": {
+ description: "The added datastore is removable.",
+ type: bool,
},
filesystem: {
type: FileSystemType,
@@ -141,7 +146,8 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
pub fn create_datastore_disk(
name: String,
disk: String,
- add_datastore: Option<bool>,
+ add_datastore: bool,
+ removable_datastore: bool,
filesystem: Option<FileSystemType>,
rpcenv: &mut dyn RpcEnvironment,
) -> Result<String, Error> {
@@ -155,8 +161,51 @@ pub fn create_datastore_disk(
bail!("disk '{}' is already in use.", disk);
}
- let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
+ if add_datastore && removable_datastore {
+ let upid_str = WorkerTask::new_thread(
+ "dircreate",
+ Some(name.clone()),
+ auth_id,
+ to_stdout,
+ move |_worker| {
+ info!("create removable datastore '{name}' on disk {disk}");
+
+ let filesystem = filesystem.unwrap_or(FileSystemType::Ext4);
+
+ let manager = DiskManage::new();
+
+ let disk = manager.disk_by_name(&disk)?;
+
+ let partition = create_single_linux_partition(&disk)?;
+ create_file_system(&partition, filesystem)?;
+
+ let uuid = get_fs_uuid(&partition)?;
+
+ let lock = pbs_config::datastore::lock_config()?;
+ let datastore: DataStoreConfig = serde_json::from_value(
+ json!({ "name": name, "path": name, "backing-device": uuid }),
+ )?;
+
+ let (config, _digest) = pbs_config::datastore::config()?;
+ if config.sections.get(&datastore.name).is_some() {
+ bail!("datastore '{}' already exists.", datastore.name);
+ }
+
+ // we don't have to check if the UUID is already in use since we just created the
+ // fs ourself
+
+ crate::api2::config::datastore::do_create_datastore(
+ lock, config, datastore, false,
+ )?;
+
+ Ok(())
+ },
+ )?;
+ return Ok(upid_str);
+ };
+
+ let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
// check if the default path exists already.
// bail if it is not empty or another filesystem mounted on top
let default_path = std::path::PathBuf::from(&mount_point);
@@ -183,7 +232,6 @@ pub fn create_datastore_disk(
move |_worker| {
info!("create datastore '{name}' on disk {disk}");
- let add_datastore = add_datastore.unwrap_or(false);
let filesystem = filesystem.unwrap_or(FileSystemType::Ext4);
let manager = DiskManage::new();
@@ -248,8 +296,9 @@ pub fn delete_datastore_disk(name: String) -> Result<(), Error> {
// path of datastore cannot be changed
let (config, _) = pbs_config::datastore::config()?;
let datastores: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
- let conflicting_datastore: Option<DataStoreConfig> =
- datastores.into_iter().find(|ds| ds.absolute_path() == path);
+ let conflicting_datastore: Option<DataStoreConfig> = datastores.into_iter().find(|ds| {
+ ds.absolute_path() == path || ds.get_mount_point().map_or(false, |mp| mp == path)
+ });
if let Some(conflicting_datastore) = conflicting_datastore {
bail!(
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 22/26] api: node: allow creation of removable datastore through directory endpoint
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 22/26] api: node: allow creation of removable datastore through directory endpoint Hannes Laimer
@ 2024-11-21 14:51 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:51 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/api2/node/disks/directory.rs | 59 +++++++++++++++++++++++++++++---
> 1 file changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 7f540220..7e020e27 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -123,6 +123,11 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
> description: "Configure a datastore using the directory.",
> type: bool,
> optional: true,
> + default: false,
> + },
> + "removable-datastore": {
> + description: "The added datastore is removable.",
> + type: bool,
> },
> filesystem: {
> type: FileSystemType,
> @@ -141,7 +146,8 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
> pub fn create_datastore_disk(
> name: String,
> disk: String,
> - add_datastore: Option<bool>,
> + add_datastore: bool,
> + removable_datastore: bool,
> filesystem: Option<FileSystemType>,
> rpcenv: &mut dyn RpcEnvironment,
> ) -> Result<String, Error> {
> @@ -155,8 +161,51 @@ pub fn create_datastore_disk(
> bail!("disk '{}' is already in use.", disk);
> }
>
> - let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
> + if add_datastore && removable_datastore {
> + let upid_str = WorkerTask::new_thread(
> + "dircreate",
> + Some(name.clone()),
> + auth_id,
> + to_stdout,
> + move |_worker| {
> + info!("create removable datastore '{name}' on disk {disk}");
> +
> + let filesystem = filesystem.unwrap_or(FileSystemType::Ext4);
> +
> + let manager = DiskManage::new();
> +
> + let disk = manager.disk_by_name(&disk)?;
> +
> + let partition = create_single_linux_partition(&disk)?;
> + create_file_system(&partition, filesystem)?;
> +
> + let uuid = get_fs_uuid(&partition)?;
> +
> + let lock = pbs_config::datastore::lock_config()?;
> + let datastore: DataStoreConfig = serde_json::from_value(
> + json!({ "name": name, "path": name, "backing-device": uuid }),
> + )?;
> +
> + let (config, _digest) = pbs_config::datastore::config()?;
>
> + if config.sections.get(&datastore.name).is_some() {
> + bail!("datastore '{}' already exists.", datastore.name);
> + }
> +
> + // we don't have to check if the UUID is already in use since we just created the
> + // fs ourself
> +
> + crate::api2::config::datastore::do_create_datastore(
> + lock, config, datastore, false,
this is also the case for the regular non-removable datastores here, but
it also means that one bug in create_datastore was missed, and some
checks are missing (some can never fail, but nested checks would make
sense for the non-removable existing code below as well, so maybe they
should be moved out into a helper that can be re-used for that?)
> + )?;
> +
> + Ok(())
> + },
this is very similar to the code below (note shown here in the patch)
for non-removable datastores, and could easily be switched around..
> + )?;
> + return Ok(upid_str);
> + };
> +
> + let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
if this part here is skipped for removable datastores, then a single
worker thread implementation with some conditional parts can be used..
> // check if the default path exists already.
> // bail if it is not empty or another filesystem mounted on top
> let default_path = std::path::PathBuf::from(&mount_point);
> @@ -183,7 +232,6 @@ pub fn create_datastore_disk(
> move |_worker| {
> info!("create datastore '{name}' on disk {disk}");
>
> - let add_datastore = add_datastore.unwrap_or(false);
> let filesystem = filesystem.unwrap_or(FileSystemType::Ext4);
>
> let manager = DiskManage::new();
> @@ -248,8 +296,9 @@ pub fn delete_datastore_disk(name: String) -> Result<(), Error> {
> // path of datastore cannot be changed
> let (config, _) = pbs_config::datastore::config()?;
> let datastores: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
> - let conflicting_datastore: Option<DataStoreConfig> =
> - datastores.into_iter().find(|ds| ds.absolute_path() == path);
> + let conflicting_datastore: Option<DataStoreConfig> = datastores.into_iter().find(|ds| {
> + ds.absolute_path() == path || ds.get_mount_point().map_or(false, |mp| mp == path)
isn't this redundant? for removable datastores, absolute_path will match
path as well..
> + });
>
> if let Some(conflicting_datastore) = conflicting_datastore {
> bail!(
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 23/26] api: node: include removable datastores in directory list
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (21 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 22/26] api: node: allow creation of removable datastore through directory endpoint Hannes Laimer
@ 2024-11-13 15:00 ` Hannes Laimer
2024-11-21 14:54 ` Fabian Grünbichler
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 24/26] node: disks: replace BASE_MOUNT_DIR with DATASTORE_MOUNT_DIR Hannes Laimer
` (3 subsequent siblings)
26 siblings, 1 reply; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:00 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/node/disks/directory.rs | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 7e020e27..21d2bcc4 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -45,6 +45,8 @@ pub struct DatastoreMountInfo {
pub path: String,
/// The mounted device.
pub device: String,
+ /// This is removable
+ pub removable: bool,
/// File system type
pub filesystem: Option<String>,
/// Mount options
@@ -61,7 +63,7 @@ pub struct DatastoreMountInfo {
}
},
returns: {
- description: "List of systemd datastore mount units.",
+ description: "List of removable-datastore devices and systemd datastore mount units.",
type: Array,
items: {
type: DatastoreMountInfo,
@@ -100,6 +102,31 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
path: data.Where,
filesystem: data.Type,
options: data.Options,
+ removable: false,
+ });
+ }
+
+ let (config, _digest) = pbs_config::datastore::config()?;
+ let store_list: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
+
+ for item in store_list
+ .into_iter()
+ .filter(|store| store.backing_device.is_some())
+ {
+ let Some(backing_device) = item.backing_device.as_deref() else {
+ continue;
+ };
+ let Some(mount_point) = item.get_mount_point() else {
+ continue;
+ };
+ list.push(DatastoreMountInfo {
+ unitfile: "datastore config".to_string(),
+ name: item.name.clone(),
+ device: format!("/dev/disk/by-uuid/{backing_device}"),
+ path: mount_point,
+ filesystem: None,
+ options: None,
+ removable: true,
});
}
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 23/26] api: node: include removable datastores in directory list
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 23/26] api: node: include removable datastores in directory list Hannes Laimer
@ 2024-11-21 14:54 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 14:54 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/api2/node/disks/directory.rs | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 7e020e27..21d2bcc4 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -45,6 +45,8 @@ pub struct DatastoreMountInfo {
> pub path: String,
> /// The mounted device.
> pub device: String,
> + /// This is removable
> + pub removable: bool,
> /// File system type
> pub filesystem: Option<String>,
> /// Mount options
> @@ -61,7 +63,7 @@ pub struct DatastoreMountInfo {
> }
> },
> returns: {
> - description: "List of systemd datastore mount units.",
> + description: "List of removable-datastore devices and systemd datastore mount units.",
> type: Array,
> items: {
> type: DatastoreMountInfo,
> @@ -100,6 +102,31 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
> path: data.Where,
> filesystem: data.Type,
> options: data.Options,
> + removable: false,
> + });
> + }
> +
> + let (config, _digest) = pbs_config::datastore::config()?;
> + let store_list: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
> +
> + for item in store_list
> + .into_iter()
> + .filter(|store| store.backing_device.is_some())
> + {
> + let Some(backing_device) = item.backing_device.as_deref() else {
> + continue;
> + };
> + let Some(mount_point) = item.get_mount_point() else {
> + continue;
> + };
this is redundant, get_mount_point will only return Some if there's a
backing_device.. in fact, I think we can remove the get_mount_point fn
entirely, and always check for backing_device (absolute_path will then
return the mountpoint..)
there's only a few places (beside this) where we only look at
get_mount_point, and those can easily be adapted to make backing_device()
*the* single way to check if a datastore is a removable one, which makes
reasoning a lot easier..
> + list.push(DatastoreMountInfo {
> + unitfile: "datastore config".to_string(),
> + name: item.name.clone(),
> + device: format!("/dev/disk/by-uuid/{backing_device}"),
> + path: mount_point,
> + filesystem: None,
> + options: None,
> + removable: true,
> });
> }
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v13 24/26] node: disks: replace BASE_MOUNT_DIR with DATASTORE_MOUNT_DIR
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (22 preceding siblings ...)
2024-11-13 15:00 ` [pbs-devel] [PATCH proxmox-backup v13 23/26] api: node: include removable datastores in directory list Hannes Laimer
@ 2024-11-13 15:01 ` Hannes Laimer
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 25/26] ui: support create removable datastore through directory creation Hannes Laimer
` (2 subsequent siblings)
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:01 UTC (permalink / raw)
To: pbs-devel
... since they do have the same value.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/node/disks/directory.rs | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 21d2bcc4..139e753d 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -11,8 +11,8 @@ use proxmox_schema::api;
use proxmox_section_config::SectionConfigData;
use pbs_api_types::{
- DataStoreConfig, BLOCKDEVICE_NAME_SCHEMA, DATASTORE_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
- PRIV_SYS_MODIFY, UPID_SCHEMA,
+ DataStoreConfig, BLOCKDEVICE_NAME_SCHEMA, DATASTORE_MOUNT_DIR, DATASTORE_SCHEMA, NODE_SCHEMA,
+ PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
};
use crate::tools::disks::{
@@ -23,8 +23,6 @@ use crate::tools::systemd::{self, types::*};
use proxmox_rest_server::WorkerTask;
-const BASE_MOUNT_DIR: &str = "/mnt/datastore/";
-
#[api(
properties: {
"filesystem": {
@@ -91,7 +89,7 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
let name = data
.Where
- .strip_prefix(BASE_MOUNT_DIR)
+ .strip_prefix(DATASTORE_MOUNT_DIR)
.unwrap_or(&data.Where)
.to_string();
@@ -232,7 +230,7 @@ pub fn create_datastore_disk(
return Ok(upid_str);
};
- let mount_point = format!("{}{}", BASE_MOUNT_DIR, &name);
+ let mount_point = format!("{}/{}", DATASTORE_MOUNT_DIR, &name);
// check if the default path exists already.
// bail if it is not empty or another filesystem mounted on top
let default_path = std::path::PathBuf::from(&mount_point);
@@ -240,7 +238,7 @@ pub fn create_datastore_disk(
match std::fs::metadata(&default_path) {
Err(_) => {} // path does not exist
Ok(stat) => {
- let basedir_dev = std::fs::metadata(BASE_MOUNT_DIR)?.st_dev();
+ let basedir_dev = std::fs::metadata(DATASTORE_MOUNT_DIR)?.st_dev();
if stat.st_dev() != basedir_dev {
bail!("path {default_path:?} already exists and is mountpoint");
}
@@ -319,7 +317,7 @@ pub fn create_datastore_disk(
)]
/// Remove a Filesystem mounted under `/mnt/datastore/<name>`.
pub fn delete_datastore_disk(name: String) -> Result<(), Error> {
- let path = format!("{}{}", BASE_MOUNT_DIR, name);
+ let path = format!("{}/{}", DATASTORE_MOUNT_DIR, name);
// path of datastore cannot be changed
let (config, _) = pbs_config::datastore::config()?;
let datastores: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 25/26] ui: support create removable datastore through directory creation
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (23 preceding siblings ...)
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 24/26] node: disks: replace BASE_MOUNT_DIR with DATASTORE_MOUNT_DIR Hannes Laimer
@ 2024-11-13 15:01 ` Hannes Laimer
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 26/26] bin: debug: add inspect device command Hannes Laimer
2024-11-21 15:13 ` [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Fabian Grünbichler
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:01 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/node/disks/directory.rs | 2 ++
www/DirectoryList.js | 13 +++++++++++++
www/window/CreateDirectory.js | 14 ++++++++++++++
3 files changed, 29 insertions(+)
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 139e753d..36c85ccf 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -153,6 +153,8 @@ pub fn list_datastore_mounts() -> Result<Vec<DatastoreMountInfo>, Error> {
"removable-datastore": {
description: "The added datastore is removable.",
type: bool,
+ optional: true,
+ default: false,
},
filesystem: {
type: FileSystemType,
diff --git a/www/DirectoryList.js b/www/DirectoryList.js
index adefa9ab..25921a62 100644
--- a/www/DirectoryList.js
+++ b/www/DirectoryList.js
@@ -121,6 +121,19 @@ Ext.define('PBS.admin.Directorylist', {
],
columns: [
+ {
+ text: '<span class="fa fa-plug"/>',
+ flex: 0,
+ width: 35,
+ dataIndex: 'removable',
+ renderer: function(_text, _, row) {
+ if (row.data.removable) {
+ return `<i class="fa fa-check"/>`;
+ } else {
+ return '';
+ }
+ },
+ },
{
text: gettext('Path'),
dataIndex: 'path',
diff --git a/www/window/CreateDirectory.js b/www/window/CreateDirectory.js
index 6aabe21a..38d6979d 100644
--- a/www/window/CreateDirectory.js
+++ b/www/window/CreateDirectory.js
@@ -43,6 +43,20 @@ Ext.define('PBS.window.CreateDirectory', {
name: 'add-datastore',
fieldLabel: gettext('Add as Datastore'),
value: '1',
+ listeners: {
+ change(field, newValue, _oldValue) {
+ let form = field.up('form');
+ let rmBox = form.down('[name=removable-datastore]');
+
+ rmBox.setDisabled(!newValue);
+ rmBox.setValue(false);
+ },
+ },
+ },
+ {
+ xtype: 'proxmoxcheckbox',
+ name: 'removable-datastore',
+ fieldLabel: gettext('is removable'),
},
],
});
--
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] 45+ messages in thread* [pbs-devel] [PATCH proxmox-backup v13 26/26] bin: debug: add inspect device command
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (24 preceding siblings ...)
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 25/26] ui: support create removable datastore through directory creation Hannes Laimer
@ 2024-11-13 15:01 ` Hannes Laimer
2024-11-21 15:13 ` [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Fabian Grünbichler
26 siblings, 0 replies; 45+ messages in thread
From: Hannes Laimer @ 2024-11-13 15:01 UTC (permalink / raw)
To: pbs-devel
... to get information about (removable) datastores a device contains
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
new since v12.
Something like this would also make sense to somehow integrate into the
UI, making it easier to select and add existsing datastores on the
device. But since the device has to be mounted, I was not sure where it
makes sense, except maybe after selecting a device, so the `path` field
could be some sort of selection. But we'd need a new endpoint and this
can definitely also be added later, so I did not include anything in this
series.
src/bin/proxmox_backup_debug/inspect.rs | 149 ++++++++++++++++++++++++
1 file changed, 149 insertions(+)
diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs
index 28a472b0..17df09be 100644
--- a/src/bin/proxmox_backup_debug/inspect.rs
+++ b/src/bin/proxmox_backup_debug/inspect.rs
@@ -331,6 +331,151 @@ fn inspect_file(
Ok(())
}
+/// Return the count of VM, CT and host backup groups and the count of namespaces
+/// as this tuple (vm, ct, host, ns)
+fn get_basic_ds_info(path: String) -> Result<(i64, i64, i64, i64), Error> {
+ let mut vms = 0;
+ let mut cts = 0;
+ let mut hosts = 0;
+ let mut ns = 0;
+ let mut walker = WalkDir::new(path).into_iter();
+
+ while let Some(entry_result) = walker.next() {
+ let entry = entry_result?;
+ if !entry.file_type().is_dir() {
+ continue;
+ }
+
+ let Some(name) = entry.path().file_name().and_then(|a| a.to_str()) else {
+ continue;
+ };
+
+ if name == ".chunks" {
+ walker.skip_current_dir();
+ continue;
+ }
+
+ let dir_count = std::fs::read_dir(entry.path())?
+ .filter_map(Result::ok)
+ .filter(|entry| entry.path().is_dir())
+ .count() as i64;
+
+ match name {
+ "ns" => ns += dir_count,
+ "vm" => {
+ vms += dir_count;
+ walker.skip_current_dir();
+ }
+ "ct" => {
+ cts += dir_count;
+ walker.skip_current_dir();
+ }
+ "host" => {
+ hosts += dir_count;
+ walker.skip_current_dir();
+ }
+ _ => {
+ // root or ns dir
+ }
+ }
+ }
+
+ Ok((vms, cts, hosts, ns))
+}
+
+#[api(
+ input: {
+ properties: {
+ device: {
+ description: "Device path, usually /dev/...",
+ type: String,
+ },
+ "output-format": {
+ schema: OUTPUT_FORMAT,
+ optional: true,
+ },
+ }
+ }
+)]
+/// Inspect a device for possible datastores on it
+fn inspect_device(device: String, param: Value) -> Result<(), Error> {
+ let output_format = get_output_format(¶m);
+ let tmp_mount_path = format!(
+ "{}/{:x}",
+ pbs_buildcfg::rundir!("/mount"),
+ proxmox_uuid::Uuid::generate()
+ );
+
+ let default_options = proxmox_sys::fs::CreateOptions::new();
+ proxmox_sys::fs::create_path(
+ &tmp_mount_path,
+ Some(default_options.clone()),
+ Some(default_options.clone()),
+ )?;
+ let mut mount_cmd = std::process::Command::new("mount");
+ mount_cmd.arg(device.clone());
+ mount_cmd.arg(tmp_mount_path.clone());
+ proxmox_sys::command::run_command(mount_cmd, None)?;
+
+ let mut walker = WalkDir::new(tmp_mount_path.clone()).into_iter();
+
+ let mut stores = Vec::new();
+
+ let mut ds_count = 0;
+ while let Some(entry_result) = walker.next() {
+ let entry = entry_result?;
+
+ if entry.file_type().is_dir()
+ && entry
+ .file_name()
+ .to_str()
+ .map_or(false, |name| name == ".chunks")
+ {
+ let store_path = entry
+ .path()
+ .to_str()
+ .and_then(|n| n.strip_suffix("/.chunks"));
+
+ if let Some(store_path) = store_path {
+ ds_count += 1;
+ let (vm, ct, host, ns) = get_basic_ds_info(store_path.to_string())?;
+ stores.push(json!({
+ "path": store_path.strip_prefix(&tmp_mount_path).unwrap_or("???"),
+ "vm-count": vm,
+ "ct-count": ct,
+ "host-count": host,
+ "ns-count": ns,
+ }));
+ };
+
+ walker.skip_current_dir();
+ }
+ }
+
+ let mut umount_cmd = std::process::Command::new("umount");
+ umount_cmd.arg(tmp_mount_path.clone());
+ proxmox_sys::command::run_command(umount_cmd, None)?;
+ std::fs::remove_dir(std::path::Path::new(&tmp_mount_path))?;
+
+ if output_format == "text" {
+ println!("Device containes {} stores", ds_count);
+ println!("---------------");
+ for s in stores {
+ println!(
+ "Datastore at {} | VM: {}, CT: {}, HOST: {}, NS: {}",
+ s["path"], s["vm-count"], s["ct-count"], s["host-count"], s["ns-count"]
+ );
+ }
+ } else {
+ format_and_print_result(
+ &json!({"store_count": stores.len(), "stores": stores}),
+ &output_format,
+ );
+ }
+
+ Ok(())
+}
+
pub fn inspect_commands() -> CommandLineInterface {
let cmd_def = CliCommandMap::new()
.insert(
@@ -340,6 +485,10 @@ pub fn inspect_commands() -> CommandLineInterface {
.insert(
"file",
CliCommand::new(&API_METHOD_INSPECT_FILE).arg_param(&["file"]),
+ )
+ .insert(
+ "device",
+ CliCommand::new(&API_METHOD_INSPECT_DEVICE).arg_param(&["device"]),
);
cmd_def.into()
--
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] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores
2024-11-13 15:00 [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Hannes Laimer
` (25 preceding siblings ...)
2024-11-13 15:01 ` [pbs-devel] [PATCH proxmox-backup v13 26/26] bin: debug: add inspect device command Hannes Laimer
@ 2024-11-21 15:13 ` Fabian Grünbichler
2024-11-21 16:49 ` Fabian Grünbichler
26 siblings, 1 reply; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 15:13 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
needs a slight rebase, lots of smallish comments on individual patches.
there's a few typos in comments and commit messages, the latter should
be mostly caught by a spell checker when resending..
things that might be nice to address before shipping this in a (public)
package/resending a rebased version:
- the mount_status return value in the api return value would be nicer
as an enum, instead of an optional bool..
- the internal command for auto-mounting might be nicer as a standalone
executable without CLI handler/.. (but that could be done later as
well, given that it is marked as internal already - it might also
increase build times, which might be an argument against it..)
- creating a second removable datastore on a device fails, since the
create_datastore API endpoint has a wrong check (see comments on
corresponding patch)
- pre-existing: while creating a datastore, we hold a lock on the config
which can cause all sorts of operations to run into a (lock) timeout,
the lock scope could maybe be reduced (for removable datastores,
creating the chunk store might take a long time..)
- unmounting a datastore over the UI spams the log with:
6:08:17 yuna proxmox-backup-proxy[812419]: GET /api2/json/admin/datastore/removable2/status: 400 Bad Request: [client [::ffff:192.168.16.37]:47734] Removable Datastore is not mounted
- unmounting over the UI, then pulling my USB drive logged the
following:
Nov 21 16:09:04 yuna kernel: EXT4-fs (sda1): shut down requested (2)
Nov 21 16:09:04 yuna kernel: Aborting journal on device sda1-8.
Nov 21 16:09:04 yuna kernel: device offline error, dev sda, sector 29624320 op 0x1:(WRITE) flags 0x9800 phys_seg 1 prio class 0
Nov 21 16:09:04 yuna kernel: Buffer I/O error on dev sda1, logical block 3702784, lost sync page write
Nov 21 16:09:04 yuna kernel: JBD2: I/O error when updating journal superblock for sda1-8.
doesn't sound good? figured that out later, what I did was:
- create "directory" removable datastore over UI
- create second removable datastore on same device (patching out the
wrong patch mentioned above)
- remove first datastore over UI (it stayed mounted, but was removed
from config and UI!)
the failure to unmount when removing is reproducible for me
the rest is mostly code style/hygiene related, and can be done as
follow-ups if needed.
I didn't look at the UI patches, just did a cursory test drive of the
resulting UI!
On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> These patches add support for removable datastores. All removable
> datastores have a backing-device(a UUID) associated with them.
> Removable datastores work like normal ones, just that they can be
> unplugged. It is possible to create a removable datastore, sync
> backups onto it, unplug it and use it on a different PBS.
>
> The datastore path specified is relative to the root of the used
> device. Removable datastores are bind mounted to
> /mnt/datastore/<NAME>. Multiple datastores can be created on a single
> device, but only device with a single datastore on them will be
> auto-mounted.
>
> When a removable datastore is deleted and 'destroy-data' is set, the
> device has to be mounted. If 'destroy-data' is not set the datastore
> can be deleted even if the device is not present. Removable datastores
> are automatically mounted when plugged in.
>
> v13: thanks @Fabian * allow multiple datastore on devices * replace
> `is_datastore_available` by a more specific function, it is now
> removable datastore specific and won't be called for normal ones *
> replace removable/is_available in status structs with mount_state,
> which is `None` for normal datastore as it makes it less ambiguous
> what is meant * remove notion of 'available' from normal datastores
> and replace it with mounted/mount_status for removable ones, as it
> never really made sense in the first place * abort of an unmount task
> will now reset the maintanance mode * add check for race when setting
> maintenance at end of unmounting task * improve documentation and
> commit messages * remove not needed tokio::spawn * only auto mount
> devices with single datastore on them * drop ptach that added flag
> for excluding used partitions * make auto mount service not dynamic *
> add debug command to scan devices for datastores they may contain *
> rebase onto master
>
> v12: thanks @Wolfgang * use bind mounts, so now <DEVICE>/path/to/ds is
> mounted to /mnt/datastore/<NAME> this is a bit cleaner and allows for
> multiple datastores on a single device to be mounted individually, if
> we want to allow that in the future * small code improvements
>
>
> v11: * rebase onto master
>
> v10: thanks @Gabriel and @Wolfgang * make is_datastore_available more
> robust * fix a lot of wording * drop format on uuid_mount command for
> UUID * only gather_disk_stats if datastore is available * overall code
> improvements * ui: include model in partition selector * rebased onto
> master
>
> v9: * change mount point to `/mnt/datastore/<name>` * update
> "Directory" list UI * add `absolute_path()` from Dietmar's RFC *
> update docs
>
> v8: * still depends on [1] * paths for removable datastores are now
> relative to `/mnt/removable_datastore/<UUID>` * add support for
> creation of removable datastore through the "create directory"
> endpoint (last 3 patches) * update datastore creation UI * update docs
>
> v7: * depends on [1] * improve logging when waiting for tasks * drop
> `update-datatore-cache` refactoring
> * fix some commit messages
>
> [1] https://lists.proxmox.com/pipermail/pbs-devel/2024-April/008739.html
>
> v6:
> * remove 'drop' flag in datastore cache
> * use maintenance-mode 'unmount' for unmounting process, only for the
> unmounting not for being unmounted
> * rename/simplify update-datastore-cache command
> * ui: integrate new unmounting maintenance mode
> * basically a mix of v3 and v4
>
> v5: thanks @Dietmar and @Christian
> * drop --force for unmount since it'll always fail if tasks are still running, and if
> there are not normal unount will work
> * improve several commit messages
> * improve error message wording
> * add removable datastore section to docs
> * add documentation for is_datastore_available
>
> v4: thanks a lot @Dietmar and @Christian
> * make check if mounted wayyy faster
> * don't keep track of mounting state
> * drop Unplugged maintenance mode
> * use UUID_FORMAT for uuid field
> * a lot of small things, like use of bail!, inline format!, ...
> * include improvement to cache handling
>
> v3:
> * remove lazy unmounting (since 9cba51ac782d04085c0af55128f32178e5132358 is applied)
> * fix CLI (un)mount command, thanks @Gabriel
> * add removable datastore CLI autocomplete helper
> * rebase onto master
> * move ui patches to the end
>
> thanks @Lukas and @Thomas for the feedback
> v2:
> * fix datastore 'add' button in the UI
> * some format!("{}", a) -> format!("{a}")
> * replace `const` with `let` in js code
> * change icon `fa-usb` -> `fa-plug`
> * add some docs
> * add JDoc for parseMaintenanceMode
> * proxmox-schema dep bump
>
> Dietmar Maurer (2):
> config: factor out method to get the absolute datastore path
> maintenance: add 'Unmount' maintenance type
>
> Hannes Laimer (24):
> tools: add disks utility functions
> pbs-api-types: add backing-device to DataStoreConfig
> disks: add UUID to partition info
> datastore: add helper for checking if a datastore is mounted
> api: admin: add (un)mount endpoint for removable datastores
> api: removable datastore creation
> pbs-api-types: add mount_status field to DataStoreListItem
> bin: manager: add (un)mount command
> add auto-mounting for removable datastores
> datastore: handle deletion of removable datastore properly
> docs: add removable datastores section
> ui: add partition selector form
> ui: add removable datastore creation support
> ui: add (un)mount button to summary
> ui: tree: render unmounted datastores correctly
> ui: utils: make parseMaintenanceMode more robust
> ui: add datastore status mask for unmounted removable datastores
> ui: maintenance: fix disable msg field if no type is selected
> ui: render 'unmount' maintenance mode correctly
> api: node: allow creation of removable datastore through directory
> endpoint
> api: node: include removable datastores in directory list
> node: disks: replace BASE_MOUNT_DIR with DATASTORE_MOUNT_DIR
> ui: support create removable datastore through directory creation
> bin: debug: add inspect device command
>
> debian/proxmox-backup-server.install | 1 +
> debian/proxmox-backup-server.udev | 3 +
> docs/storage.rst | 38 +++
> etc/Makefile | 3 +-
> etc/removable-device-attach@.service | 8 +
> pbs-api-types/src/datastore.rs | 46 +++-
> pbs-api-types/src/maintenance.rs | 7 +-
> pbs-config/src/datastore.rs | 14 +
> pbs-datastore/src/datastore.rs | 88 +++++-
> pbs-datastore/src/lib.rs | 2 +-
> src/api2/admin/datastore.rs | 289 ++++++++++++++++++--
> src/api2/config/datastore.rs | 87 +++++-
> src/api2/node/disks/directory.rs | 104 ++++++-
> src/api2/status/mod.rs | 29 +-
> src/bin/proxmox_backup_debug/inspect.rs | 149 ++++++++++
> src/bin/proxmox_backup_manager/datastore.rs | 136 ++++++++-
> src/server/metric_collection/mod.rs | 18 +-
> src/tools/disks/mod.rs | 39 ++-
> www/DirectoryList.js | 13 +
> www/Makefile | 1 +
> www/NavigationTree.js | 17 +-
> www/Utils.js | 33 ++-
> www/css/ext6-pbs.css | 20 ++
> www/datastore/DataStoreListSummary.js | 1 +
> www/datastore/Summary.js | 113 +++++++-
> www/form/PartitionSelector.js | 81 ++++++
> www/window/CreateDirectory.js | 14 +
> www/window/DataStoreEdit.js | 37 +++
> www/window/MaintenanceOptions.js | 17 +-
> 29 files changed, 1328 insertions(+), 80 deletions(-)
> create mode 100644 etc/removable-device-attach@.service
> create mode 100644 www/form/PartitionSelector.js
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores
2024-11-21 15:13 ` [pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores Fabian Grünbichler
@ 2024-11-21 16:49 ` Fabian Grünbichler
0 siblings, 0 replies; 45+ messages in thread
From: Fabian Grünbichler @ 2024-11-21 16:49 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Hannes Laimer
one more thing: it probably makes sense to additionally require Sys.Modify on /system/disks for paths adding/(modifying?)/removing removable datastores, to mimic the permissions required for adding a directory-backed managed datastore..
> Fabian Grünbichler <f.gruenbichler@proxmox.com> hat am 21.11.2024 16:13 CET geschrieben:
>
>
> needs a slight rebase, lots of smallish comments on individual patches.
> there's a few typos in comments and commit messages, the latter should
> be mostly caught by a spell checker when resending..
>
> things that might be nice to address before shipping this in a (public)
> package/resending a rebased version:
>
> - the mount_status return value in the api return value would be nicer
> as an enum, instead of an optional bool..
> - the internal command for auto-mounting might be nicer as a standalone
> executable without CLI handler/.. (but that could be done later as
> well, given that it is marked as internal already - it might also
> increase build times, which might be an argument against it..)
> - creating a second removable datastore on a device fails, since the
> create_datastore API endpoint has a wrong check (see comments on
> corresponding patch)
> - pre-existing: while creating a datastore, we hold a lock on the config
> which can cause all sorts of operations to run into a (lock) timeout,
> the lock scope could maybe be reduced (for removable datastores,
> creating the chunk store might take a long time..)
> - unmounting a datastore over the UI spams the log with:
>
> 6:08:17 yuna proxmox-backup-proxy[812419]: GET /api2/json/admin/datastore/removable2/status: 400 Bad Request: [client [::ffff:192.168.16.37]:47734] Removable Datastore is not mounted
>
> - unmounting over the UI, then pulling my USB drive logged the
> following:
>
> Nov 21 16:09:04 yuna kernel: EXT4-fs (sda1): shut down requested (2)
> Nov 21 16:09:04 yuna kernel: Aborting journal on device sda1-8.
> Nov 21 16:09:04 yuna kernel: device offline error, dev sda, sector 29624320 op 0x1:(WRITE) flags 0x9800 phys_seg 1 prio class 0
> Nov 21 16:09:04 yuna kernel: Buffer I/O error on dev sda1, logical block 3702784, lost sync page write
> Nov 21 16:09:04 yuna kernel: JBD2: I/O error when updating journal superblock for sda1-8.
>
> doesn't sound good? figured that out later, what I did was:
>
> - create "directory" removable datastore over UI
> - create second removable datastore on same device (patching out the
> wrong patch mentioned above)
> - remove first datastore over UI (it stayed mounted, but was removed
> from config and UI!)
>
> the failure to unmount when removing is reproducible for me
>
> the rest is mostly code style/hygiene related, and can be done as
> follow-ups if needed.
>
> I didn't look at the UI patches, just did a cursory test drive of the
> resulting UI!
>
> On November 13, 2024 4:00 pm, Hannes Laimer wrote:
> > These patches add support for removable datastores. All removable
> > datastores have a backing-device(a UUID) associated with them.
> > Removable datastores work like normal ones, just that they can be
> > unplugged. It is possible to create a removable datastore, sync
> > backups onto it, unplug it and use it on a different PBS.
> >
> > The datastore path specified is relative to the root of the used
> > device. Removable datastores are bind mounted to
> > /mnt/datastore/<NAME>. Multiple datastores can be created on a single
> > device, but only device with a single datastore on them will be
> > auto-mounted.
> >
> > When a removable datastore is deleted and 'destroy-data' is set, the
> > device has to be mounted. If 'destroy-data' is not set the datastore
> > can be deleted even if the device is not present. Removable datastores
> > are automatically mounted when plugged in.
> >
> > v13: thanks @Fabian * allow multiple datastore on devices * replace
> > `is_datastore_available` by a more specific function, it is now
> > removable datastore specific and won't be called for normal ones *
> > replace removable/is_available in status structs with mount_state,
> > which is `None` for normal datastore as it makes it less ambiguous
> > what is meant * remove notion of 'available' from normal datastores
> > and replace it with mounted/mount_status for removable ones, as it
> > never really made sense in the first place * abort of an unmount task
> > will now reset the maintanance mode * add check for race when setting
> > maintenance at end of unmounting task * improve documentation and
> > commit messages * remove not needed tokio::spawn * only auto mount
> > devices with single datastore on them * drop ptach that added flag
> > for excluding used partitions * make auto mount service not dynamic *
> > add debug command to scan devices for datastores they may contain *
> > rebase onto master
> >
> > v12: thanks @Wolfgang * use bind mounts, so now <DEVICE>/path/to/ds is
> > mounted to /mnt/datastore/<NAME> this is a bit cleaner and allows for
> > multiple datastores on a single device to be mounted individually, if
> > we want to allow that in the future * small code improvements
> >
> >
> > v11: * rebase onto master
> >
> > v10: thanks @Gabriel and @Wolfgang * make is_datastore_available more
> > robust * fix a lot of wording * drop format on uuid_mount command for
> > UUID * only gather_disk_stats if datastore is available * overall code
> > improvements * ui: include model in partition selector * rebased onto
> > master
> >
> > v9: * change mount point to `/mnt/datastore/<name>` * update
> > "Directory" list UI * add `absolute_path()` from Dietmar's RFC *
> > update docs
> >
> > v8: * still depends on [1] * paths for removable datastores are now
> > relative to `/mnt/removable_datastore/<UUID>` * add support for
> > creation of removable datastore through the "create directory"
> > endpoint (last 3 patches) * update datastore creation UI * update docs
> >
> > v7: * depends on [1] * improve logging when waiting for tasks * drop
> > `update-datatore-cache` refactoring
> > * fix some commit messages
> >
> > [1] https://lists.proxmox.com/pipermail/pbs-devel/2024-April/008739.html
> >
> > v6:
> > * remove 'drop' flag in datastore cache
> > * use maintenance-mode 'unmount' for unmounting process, only for the
> > unmounting not for being unmounted
> > * rename/simplify update-datastore-cache command
> > * ui: integrate new unmounting maintenance mode
> > * basically a mix of v3 and v4
> >
> > v5: thanks @Dietmar and @Christian
> > * drop --force for unmount since it'll always fail if tasks are still running, and if
> > there are not normal unount will work
> > * improve several commit messages
> > * improve error message wording
> > * add removable datastore section to docs
> > * add documentation for is_datastore_available
> >
> > v4: thanks a lot @Dietmar and @Christian
> > * make check if mounted wayyy faster
> > * don't keep track of mounting state
> > * drop Unplugged maintenance mode
> > * use UUID_FORMAT for uuid field
> > * a lot of small things, like use of bail!, inline format!, ...
> > * include improvement to cache handling
> >
> > v3:
> > * remove lazy unmounting (since 9cba51ac782d04085c0af55128f32178e5132358 is applied)
> > * fix CLI (un)mount command, thanks @Gabriel
> > * add removable datastore CLI autocomplete helper
> > * rebase onto master
> > * move ui patches to the end
> >
> > thanks @Lukas and @Thomas for the feedback
> > v2:
> > * fix datastore 'add' button in the UI
> > * some format!("{}", a) -> format!("{a}")
> > * replace `const` with `let` in js code
> > * change icon `fa-usb` -> `fa-plug`
> > * add some docs
> > * add JDoc for parseMaintenanceMode
> > * proxmox-schema dep bump
> >
> > Dietmar Maurer (2):
> > config: factor out method to get the absolute datastore path
> > maintenance: add 'Unmount' maintenance type
> >
> > Hannes Laimer (24):
> > tools: add disks utility functions
> > pbs-api-types: add backing-device to DataStoreConfig
> > disks: add UUID to partition info
> > datastore: add helper for checking if a datastore is mounted
> > api: admin: add (un)mount endpoint for removable datastores
> > api: removable datastore creation
> > pbs-api-types: add mount_status field to DataStoreListItem
> > bin: manager: add (un)mount command
> > add auto-mounting for removable datastores
> > datastore: handle deletion of removable datastore properly
> > docs: add removable datastores section
> > ui: add partition selector form
> > ui: add removable datastore creation support
> > ui: add (un)mount button to summary
> > ui: tree: render unmounted datastores correctly
> > ui: utils: make parseMaintenanceMode more robust
> > ui: add datastore status mask for unmounted removable datastores
> > ui: maintenance: fix disable msg field if no type is selected
> > ui: render 'unmount' maintenance mode correctly
> > api: node: allow creation of removable datastore through directory
> > endpoint
> > api: node: include removable datastores in directory list
> > node: disks: replace BASE_MOUNT_DIR with DATASTORE_MOUNT_DIR
> > ui: support create removable datastore through directory creation
> > bin: debug: add inspect device command
> >
> > debian/proxmox-backup-server.install | 1 +
> > debian/proxmox-backup-server.udev | 3 +
> > docs/storage.rst | 38 +++
> > etc/Makefile | 3 +-
> > etc/removable-device-attach@.service | 8 +
> > pbs-api-types/src/datastore.rs | 46 +++-
> > pbs-api-types/src/maintenance.rs | 7 +-
> > pbs-config/src/datastore.rs | 14 +
> > pbs-datastore/src/datastore.rs | 88 +++++-
> > pbs-datastore/src/lib.rs | 2 +-
> > src/api2/admin/datastore.rs | 289 ++++++++++++++++++--
> > src/api2/config/datastore.rs | 87 +++++-
> > src/api2/node/disks/directory.rs | 104 ++++++-
> > src/api2/status/mod.rs | 29 +-
> > src/bin/proxmox_backup_debug/inspect.rs | 149 ++++++++++
> > src/bin/proxmox_backup_manager/datastore.rs | 136 ++++++++-
> > src/server/metric_collection/mod.rs | 18 +-
> > src/tools/disks/mod.rs | 39 ++-
> > www/DirectoryList.js | 13 +
> > www/Makefile | 1 +
> > www/NavigationTree.js | 17 +-
> > www/Utils.js | 33 ++-
> > www/css/ext6-pbs.css | 20 ++
> > www/datastore/DataStoreListSummary.js | 1 +
> > www/datastore/Summary.js | 113 +++++++-
> > www/form/PartitionSelector.js | 81 ++++++
> > www/window/CreateDirectory.js | 14 +
> > www/window/DataStoreEdit.js | 37 +++
> > www/window/MaintenanceOptions.js | 17 +-
> > 29 files changed, 1328 insertions(+), 80 deletions(-)
> > create mode 100644 etc/removable-device-attach@.service
> > create mode 100644 www/form/PartitionSelector.js
> >
> > --
> > 2.39.5
> >
> >
> >
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> >
> >
> >
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 45+ messages in thread