From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C9CC296F8C for ; Tue, 16 Apr 2024 17:04:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B354E1E048 for ; Tue, 16 Apr 2024 17:04:03 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 16 Apr 2024 17:03:57 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 09DAE4528B for ; Tue, 16 Apr 2024 17:03:57 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 16 Apr 2024 17:03:51 +0200 Message-Id: From: "Hannes Laimer" To: "Christian Ebner" , "Proxmox Backup Server development discussion" X-Mailer: aerc 0.17.0-78-g4ffbaa6b3946 References: <20240409110012.166472-1-h.laimer@proxmox.com> <61795eec-ca5d-4aa7-9a26-87704469330a@proxmox.com> In-Reply-To: <61795eec-ca5d-4aa7-9a26-87704469330a@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.008 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 00/24] add removable datastores X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Apr 2024 15:04:03 -0000 On Tue Apr 16, 2024 at 11:37 AM CEST, Christian Ebner wrote: > Hi, > > had a look a this patch series (mostly at the Rust code, did not feel=20 > competent enough to comment to much on the ExtJS side). > > All in all this looks nice and adds a feature requested by quite some=20 > users already. > I did however missed a flag to setup a datastore directly as removable=20 > when setting it up via the disk management interface instead of going=20 > through the datastore create dialog. This would be nice to have if not=20 > to complex to implement. yes, and shouldn't be too complicated it should mostly be UI stuff, but won= 't be in v4. > Also, the current approach with the flag switching from the path field=20 > to the disk selector did not feel very intuitive on first usage. Maybe=20 > both could be present all the time and only enabled/disabled=20 > accordingly. Would also allow to keep the path if I already gave one but= =20 > clicked on the checkbox after that. the path is generated based on the selected partition, but sure, could still make sense to have both fields visible. > Also, the error message when navigating to the datastores contents tab=20 > while the store being unplugged needs rephrasing I think. Or even show=20 > the unplugged state as in the summary instead. > > If the comments are addressed, please consider this: > > Tested-by: Christian Ebner > Reviewed-by: Christian Ebner > > Thanks a lot for looking over the series! v4 will be structured a little different but I'll include still relevant (which are almost all) suggeststions. > On 4/9/24 12:59, Hannes Laimer wrote: > > These patches add support for removable datastores. All removable > > datastores have a backing-device(a UUID) associated with them. Removabl= e > > datastores work like normal ones, just that they can be unplugged. It i= s > > possible to create a removable datastore, sync backups onto it, unplug > > it and use it on a different PBS. > >=20 > > The datastore path is also the mountpoint for the removable datastore. > > By default when creating one through the web UI it will be > > `/mnt/removable-datastores/`, using the CLI it is possible to > > specify something else. Since a removable datastore is associated with > > the UUID of a partition, it is technically possible to have two > > removable datastores on the same device, but I don't think there is a > > use-case that couldn't also be done using namespaces. > >=20 > > When a removable datastore is deleted and 'destroy-data' is set, the > > device has to be plugged 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. At the API service start all > > removable datastores are marked as 'unplugged', unless they are already > > mounted. > >=20 > > Patches 21/24 and 23/24 are not strictly needed, but they made sense in > > this context, so I kept them in this series. > >=20 > > =20 > > v3: > > * remove lazy unmounting (since 9cba51ac782d04085c0af55128f32178e5132= 358 is applied) > > * fix CLI (un)mount command, thanks @Gabriel > > * add removable datastore CLI autocomplete helper > > * rebase onto master > > * move ui patches to the end > >=20 > > 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 > >=20 > > Hannes Laimer (24): > > tools: add disks utility functions > > pbs-api-types: add backing-device to DataStoreConfig > > maintenance: add 'Unpplugged' maintenance type > > disks: add UUID to partition info > > add helper for checking if a removable datastore is available > > api2: admin: add (un)mount endpoint for removable datastores > > api2: removable datastore creation > > api2: disks list: add only-unused flag > > pbs-api-types: datastore: use new proxmox_scheme::de for > > deserialization > > pbs-api-types: add removable/is-available flag to DataStoreListItem > > pb-manager: add (un)mount command > > add auto-mounting for removable datastores > > api: mark removable datastores as unplugged after restart > > datastore: handle deletion of removable datastore properly > > docs: mention maintenance mode reset when removable datastore is > > unplugged > > ui: add partition selector form > > ui: add removable datastore creation support > > ui: add (un)mount button to summary > > ui: display removable datastores in list > > ui: utils: render unplugged maintenance mode correctly > > ui: utils: make parseMaintenanceMode more robust > > ui: add datastore status mask for unplugged removable datastores > > ui: maintenance: fix disable msg field if no type is selected > > ui: maintenance: disable edit if unplugged > >=20 > > debian/proxmox-backup-server.install | 1 + > > debian/proxmox-backup-server.udev | 3 + > > docs/maintenance.rst | 2 + > > etc/Makefile | 3 +- > > etc/removable-device-attach@.service.in | 8 + > > pbs-api-types/src/datastore.rs | 40 ++++- > > pbs-api-types/src/maintenance.rs | 35 +++- > > pbs-config/src/datastore.rs | 14 ++ > > pbs-datastore/src/datastore.rs | 28 ++- > > pbs-datastore/src/lib.rs | 2 +- > > src/api2/admin/datastore.rs | 190 +++++++++++++++++++= - > > src/api2/config/datastore.rs | 67 ++++++- > > src/api2/node/disks/mod.rs | 8 + > > src/api2/status.rs | 18 +- > > src/bin/proxmox-backup-api.rs | 18 ++ > > src/bin/proxmox_backup_manager/datastore.rs | 131 +++++++++++++- > > src/tools/disks/mod.rs | 95 ++++++++-- > > www/Makefile | 1 + > > www/NavigationTree.js | 10 +- > > www/Utils.js | 31 +++- > > www/css/ext6-pbs.css | 20 +++ > > www/datastore/DataStoreListSummary.js | 1 + > > www/datastore/Summary.js | 106 +++++++++-- > > www/form/PartitionSelector.js | 59 ++++++ > > www/window/DataStoreEdit.js | 51 ++++++ > > www/window/MaintenanceOptions.js | 16 +- > > 26 files changed, 899 insertions(+), 59 deletions(-) > > create mode 100644 etc/removable-device-attach@.service.in > > create mode 100644 www/form/PartitionSelector.js > >=20