Re: [pbs-devel] [PATCH proxmox-backup v10 00/26] add removable datastores

["Max Carrara" <m.carrara@proxmox.com>]

On Thu Apr 25, 2024 at 8:52 AM CEST, 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.

Looks pretty solid! Applied the patches and gave it a spin on my test
instance. There are some more comments inline, mostly suggesting to use
our own UUID type from `proxmox_uuid` instead of plain strings.
Otherwise, see the sections below.

Testing
=======

* Attached a new disk to my VM
* Created a new removable datastore via the Storage / Disks menu
* Configured the new datastore in my host's storage
* Ran a couple backups
* Set up Prune, GC and Verify jobs
* Set up Sync job to pull from other local datastore
* Manually unmounted and mounted the datastore, UI reacted smoothly
  (very dank actually)

Now to the more fun part: I decided to ignore the docs and detach the
(SCSI) disk I set the datastore up on in the GUI of my host. Lo and
behold, the datastore was still unmounted correctly; at least the UI
didn't complain. As soon as I attached the disk again, the datastore was
almost immediately found. Very robust, I like it!

Code Review
===========

I've got nothing to complain - apart from the comments inline regarding
UUIDs. The vast majority of patches are very easy to follow, each change
is kept comprehensible. The context of the commits is very clear - no
commit tries to do too many things at once. Very good work!

Regarding Failed Jobs
=====================

One thing I noticed is that jobs will still run and consequently fail if
the datastore is unmounted. I guess that's behaving as expected, but if
I put myself in the user's shoes, I'd prefer those jobs to not run at
all instead of failing on every run.

If I (for example) have a job that runs every hour, that would amount to
24 errors a day, which might be a lot of noise - if I have 168 failed
jobs in a week and some other e.g. *very important* job happens to fail
some time inbetween, that very important job now gets lost between those
(IMO) unnecessary fails.

That's IMHO the only thing that requires some more attention
(disclaimer: I'm not sure if there's been some discussion about this
already); I've got no complaints otherwise. Pretty solid work I gotta
say!

Bikeshedding
============

The lesser important thing I wanted to bring up is: Is "Removable
Datastore" really the right name? I would personally prefer something
like "Pluggable Datastore" or something similar, because all datastores
can be removed - it's just that the features added in this series allows
*some* of them to be re-attached again.
  
Specifically, there's a "Remove Datastore" button already in the options
section of datastores; I think the name would conflict with the meaning
of this button. (Sure, you could argue that it's context-dependent, but
I still don't think that those should clash.)

>
> The datastore path specified is relative to `/mnt/removable_datastore/<name>`.
>
> When a removable datastore is deleted and 'destroy-data' is set, the
> device has to be mounted 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. 
>
>  
> 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 removable datastore is
>     available
>   api: admin: add (un)mount endpoint for removable datastores
>   api: removable datastore creation
>   api: disks list: add exclude-used flag
>   pbs-api-types: add removable/is-available flag 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
>
>  debian/proxmox-backup-server.install        |   1 +
>  debian/proxmox-backup-server.udev           |   3 +
>  docs/storage.rst                            |  29 +++
>  etc/Makefile                                |   3 +-
>  etc/removable-device-attach@.service.in     |   8 +
>  pbs-api-types/src/datastore.rs              |  42 +++-
>  pbs-api-types/src/maintenance.rs            |  11 +-
>  pbs-config/src/datastore.rs                 |  14 ++
>  pbs-datastore/src/datastore.rs              |  79 +++++++-
>  pbs-datastore/src/lib.rs                    |   2 +-
>  src/api2/admin/datastore.rs                 | 203 ++++++++++++++++++--
>  src/api2/config/datastore.rs                |  69 ++++++-
>  src/api2/node/disks/directory.rs            | 112 +++++++++--
>  src/api2/node/disks/mod.rs                  |   8 +
>  src/api2/status.rs                          |  18 +-
>  src/bin/proxmox-backup-proxy.rs             |   9 +-
>  src/bin/proxmox_backup_manager/datastore.rs | 129 ++++++++++++-
>  src/tools/disks/mod.rs                      |  89 ++++++++-
>  www/DirectoryList.js                        |  13 ++
>  www/Makefile                                |   1 +
>  www/NavigationTree.js                       |  15 +-
>  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, 1086 insertions(+), 88 deletions(-)
>  create mode 100644 etc/removable-device-attach@.service.in
>  create mode 100644 www/form/PartitionSelector.js



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