[pbs-devel] [PATCH proxmox-backup v13 00/26] add removable datastores
Fabian Grünbichler
f.gruenbichler at proxmox.com
Thu Nov 21 17:49:37 CET 2024
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 at 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 at .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 at .service
> > create mode 100644 www/form/PartitionSelector.js
> >
> > --
> > 2.39.5
> >
> >
> >
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel at lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> >
> >
> >
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
More information about the pbs-devel
mailing list