[pbs-devel] [PATCH proxmox-backup v10 00/26] add removable datastores
Max Carrara
m.carrara at proxmox.com
Fri May 3 13:34:25 CEST 2024
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 at .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 at .service.in
> create mode 100644 www/form/PartitionSelector.js
More information about the pbs-devel
mailing list