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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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