Re: [pbs-devel] [PATCH proxmox-backup v3 00/24] add removable datastores

[Christian Ebner <c.ebner@proxmox.com>]

Hi,

had a look a this patch series (mostly at the Rust code, did not feel 
competent enough to comment to much on the ExtJS side).

All in all this looks nice and adds a feature requested by quite some 
users already.
I did however missed a flag to setup a datastore directly as removable 
when setting it up via the disk management interface instead of going 
through the datastore create dialog. This would be nice to have if not 
to complex to implement.
Also, the current approach with the flag switching from the path field 
to the disk selector did not feel very intuitive on first usage. Maybe 
both could be present all the time and only enabled/disabled 
accordingly. Would also allow to keep the path if I already gave one but 
clicked on the checkbox after that.
Also, the error message when navigating to the datastores contents tab 
while the store being unplugged needs rephrasing I think. Or even show 
the unplugged state as in the summary instead.

If the comments are addressed, please consider this:

Tested-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>


On 4/9/24 12:59, 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 is also the mountpoint for the removable datastore.
> By default when creating one through the web UI it will be
> `/mnt/removable-datastores/<UUID>`, using the CLI it is possible to
> specify something else. Since a removable datastore is associated with
> the UUID of a partition, it is technically possible to have two
> removable datastores on the same device, but I don't think there is a
> use-case that couldn't also be done using namespaces.
> 
> When a removable datastore is deleted and 'destroy-data' is set, the
> device has to be plugged in. 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. At the API service start all
> removable datastores are marked as 'unplugged', unless they are already
> mounted.
> 
> Patches 21/24 and 23/24 are not strictly needed, but they made sense in
> this context, so I kept them in this series.
> 
>   
> 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
> 
> Hannes Laimer (24):
>    tools: add disks utility functions
>    pbs-api-types: add backing-device to DataStoreConfig
>    maintenance: add 'Unpplugged' maintenance type
>    disks: add UUID to partition info
>    add helper for checking if a removable datastore is available
>    api2: admin: add (un)mount endpoint for removable datastores
>    api2: removable datastore creation
>    api2: disks list: add only-unused flag
>    pbs-api-types: datastore: use new proxmox_scheme::de for
>      deserialization
>    pbs-api-types: add removable/is-available flag to DataStoreListItem
>    pb-manager: add (un)mount command
>    add auto-mounting for removable datastores
>    api: mark removable datastores as unplugged after restart
>    datastore: handle deletion of removable datastore properly
>    docs: mention maintenance mode reset when removable datastore is
>      unplugged
>    ui: add partition selector form
>    ui: add removable datastore creation support
>    ui: add (un)mount button to summary
>    ui: display removable datastores in list
>    ui: utils: render unplugged maintenance mode correctly
>    ui: utils: make parseMaintenanceMode more robust
>    ui: add datastore status mask for unplugged removable datastores
>    ui: maintenance: fix disable msg field if no type is selected
>    ui: maintenance: disable edit if unplugged
> 
>   debian/proxmox-backup-server.install        |   1 +
>   debian/proxmox-backup-server.udev           |   3 +
>   docs/maintenance.rst                        |   2 +
>   etc/Makefile                                |   3 +-
>   etc/removable-device-attach@.service.in     |   8 +
>   pbs-api-types/src/datastore.rs              |  40 ++++-
>   pbs-api-types/src/maintenance.rs            |  35 +++-
>   pbs-config/src/datastore.rs                 |  14 ++
>   pbs-datastore/src/datastore.rs              |  28 ++-
>   pbs-datastore/src/lib.rs                    |   2 +-
>   src/api2/admin/datastore.rs                 | 190 +++++++++++++++++++-
>   src/api2/config/datastore.rs                |  67 ++++++-
>   src/api2/node/disks/mod.rs                  |   8 +
>   src/api2/status.rs                          |  18 +-
>   src/bin/proxmox-backup-api.rs               |  18 ++
>   src/bin/proxmox_backup_manager/datastore.rs | 131 +++++++++++++-
>   src/tools/disks/mod.rs                      |  95 ++++++++--
>   www/Makefile                                |   1 +
>   www/NavigationTree.js                       |  10 +-
>   www/Utils.js                                |  31 +++-
>   www/css/ext6-pbs.css                        |  20 +++
>   www/datastore/DataStoreListSummary.js       |   1 +
>   www/datastore/Summary.js                    | 106 +++++++++--
>   www/form/PartitionSelector.js               |  59 ++++++
>   www/window/DataStoreEdit.js                 |  51 ++++++
>   www/window/MaintenanceOptions.js            |  16 +-
>   26 files changed, 899 insertions(+), 59 deletions(-)
>   create mode 100644 etc/removable-device-attach@.service.in
>   create mode 100644 www/form/PartitionSelector.js
>