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 0E92496720 for ; Tue, 16 Apr 2024 11:37:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E25831727D for ; Tue, 16 Apr 2024 11:37:18 +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 11:37:18 +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 EB77644DBC for ; Tue, 16 Apr 2024 11:37:17 +0200 (CEST) Message-ID: <61795eec-ca5d-4aa7-9a26-87704469330a@proxmox.com> Date: Tue, 16 Apr 2024 11:37:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20240409110012.166472-1-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20240409110012.166472-1-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.032 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 09:37:19 -0000 Hi, had a look a this patch series (mostly at the Rust code, did not feel competent enough to comment to much on the ExtJS side). All in all this looks nice and adds a feature requested by quite some users already. I did however missed a flag to setup a datastore directly as removable when setting it up via the disk management interface instead of going through the datastore create dialog. This would be nice to have if not to complex to implement. Also, the current approach with the flag switching from the path field to the disk selector did not feel very intuitive on first usage. Maybe both could be present all the time and only enabled/disabled accordingly. Would also allow to keep the path if I already gave one but clicked on the checkbox after that. Also, the error message when navigating to the datastores contents tab while the store being unplugged needs rephrasing I think. Or even show the unplugged state as in the summary instead. If the comments are addressed, please consider this: Tested-by: Christian Ebner Reviewed-by: Christian Ebner 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. 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 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. > > 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. > > Patches 21/24 and 23/24 are not strictly needed, but they made sense in > this context, so I kept them in this series. > > > 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 > > 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 > > 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 >