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

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

one more thing: it probably makes sense to additionally require Sys.Modify on /system/disks for paths adding/(modifying?)/removing removable datastores, to mimic the permissions required for adding a directory-backed managed datastore..

> Fabian Grünbichler <f.gruenbichler@proxmox.com> hat am 21.11.2024 16:13 CET geschrieben:
> 
>  
> 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


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