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

["Hannes Laimer" <h.laimer@proxmox.com>]

On Tue Apr 16, 2024 at 11:37 AM CEST, Christian Ebner wrote:
> 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.

yes, and shouldn't be too complicated it should mostly be UI stuff, but won't be
in v4.

> 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.

the path is generated based on the selected partition, but sure, could
still make sense to have both fields visible.

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

Thanks a lot for looking over the series! v4 will be structured a little
different but I'll include still relevant (which are almost all)
suggeststions.

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