Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dietmar Maurer]

> > Hmm, what would you do if more than one of them are plugged in at the same time?
> > Maybe hard to time on a running system, but possible if I connect a hub with
> > devices already plugged in, and can definitively happen (by mistake) on cold boot.
> > 
> 
> my guess is that the udev rules do not trigger simultaneously ?

We detect both devices ...

> and even if, we could lock the mount/unmount so that this
> does not make a problem and we simply mount the first
> triggered one

Why not simply sync to both disks?

[pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Hannes Laimer]

v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
 - do not allow creation of removable datastores on a device that 
   is mounted at '/'
 - ui: also show the mount point in the Options-tab

Adds the possibility to create removable datastores. A removable
datastore has a UUID(the device on which the data is stored) and a
mount-point(where the device will be mounted), iff both are set the
datastore is removable. Everything else is identical to normal
datastores. Since config files for jobs, etc. are stored on the server,
all configuration can be done with the device plugged in or not. Certain
statistics about the datastore won't be available as long as it is not
plugged in. 
Removable datastores have to be unmounted before removing, it can only 
be unmounted if not jibs are running.
Removable datastores are mounted automatically when the device is plugged in, if it has
been unmounted, it has to be mounted manually through the WebUI or the Api.
Jobs will not be started if the datastore is not available, and
depending on the configuration, start when the device is plugged in the
next time.

Still todo:
 - make sync to local datastore more integrated
 - (add 'when plugged in'-option to job schedule?)
 - replace linux commands with internal functions in tools/disks, where
    possible

Hannes Laimer (15):
  tools: add disks utility functions
  config: add uuid+mountpoint to DataStoreConfig
  api2: add support for removable datastore creation
  backup: add check_if_available function to ds
  api2: add 'is_available' to DataStoreConfig
  api2: add 'removable' to DataStoreListItem
  api2: add (un)mount endpoint for removable ds's
  pbs: add mount-removable command to commandSocket
  pbs-manager: add 'send-command' command
  debian: add udev rule for removable datastores
  ui: show usb icon for removable datastore in list
  ui: add 'removable' checkbox in datastore creation
  ui: display row as disabled in ds statistics
  ui: show backing device UUID and mount-point in option tab
  ui: add (un)mount button to summary

 debian/proxmox-backup-server.udev    |   3 +
 pbs-api-types/src/lib.rs             |   7 ++
 src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
 src/api2/config/datastore.rs         |  22 +++-
 src/api2/status.rs                   |  19 +++-
 src/api2/types/mod.rs                |   2 +
 src/backup/datastore.rs              |  23 ++++
 src/backup/mod.rs                    |   2 +-
 src/bin/proxmox-backup-api.rs        |  27 +++++
 src/bin/proxmox-backup-manager.rs    |  41 +++++++
 src/config/datastore.rs              |  16 +++
 src/tools/disks/mod.rs               |  53 +++++++++
 www/NavigationTree.js                |   3 +-
 www/dashboard/DataStoreStatistics.js |   3 +
 www/datastore/OptionView.js          |   6 +
 www/datastore/Summary.js             |  77 ++++++++++++-
 www/window/DataStoreEdit.js          |   5 +
 17 files changed, 460 insertions(+), 8 deletions(-)

-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dominik Csapak]

High level comment here, rest in the relevant patches:

looks mostly ok, but one things stood out:

why do you add a command for the socket and the
manager 'send command' method? would it not have
been easier to call the mount api directly
from the proxmox-backup-manager?

also, would it not make sense to have
multiple uuids per datastore instead of only one?

i guess most users of this will want to have more than
one usb devices in rotation, and having multiple
datastores + multiple sync jobs is probably
a bit error prone. just allowing multiple
uuids for each datastore would solve that
problem nicely

On 8/30/21 13:14, Hannes Laimer wrote:
> v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
>   - do not allow creation of removable datastores on a device that
>     is mounted at '/'
>   - ui: also show the mount point in the Options-tab
> 
> Adds the possibility to create removable datastores. A removable
> datastore has a UUID(the device on which the data is stored) and a
> mount-point(where the device will be mounted), iff both are set the
> datastore is removable. Everything else is identical to normal
> datastores. Since config files for jobs, etc. are stored on the server,
> all configuration can be done with the device plugged in or not. Certain
> statistics about the datastore won't be available as long as it is not
> plugged in.
> Removable datastores have to be unmounted before removing, it can only
> be unmounted if not jibs are running.
> Removable datastores are mounted automatically when the device is plugged in, if it has
> been unmounted, it has to be mounted manually through the WebUI or the Api.
> Jobs will not be started if the datastore is not available, and
> depending on the configuration, start when the device is plugged in the
> next time.
> 
> Still todo:
>   - make sync to local datastore more integrated
>   - (add 'when plugged in'-option to job schedule?)
>   - replace linux commands with internal functions in tools/disks, where
>      possible
> 
> Hannes Laimer (15):
>    tools: add disks utility functions
>    config: add uuid+mountpoint to DataStoreConfig
>    api2: add support for removable datastore creation
>    backup: add check_if_available function to ds
>    api2: add 'is_available' to DataStoreConfig
>    api2: add 'removable' to DataStoreListItem
>    api2: add (un)mount endpoint for removable ds's
>    pbs: add mount-removable command to commandSocket
>    pbs-manager: add 'send-command' command
>    debian: add udev rule for removable datastores
>    ui: show usb icon for removable datastore in list
>    ui: add 'removable' checkbox in datastore creation
>    ui: display row as disabled in ds statistics
>    ui: show backing device UUID and mount-point in option tab
>    ui: add (un)mount button to summary
> 
>   debian/proxmox-backup-server.udev    |   3 +
>   pbs-api-types/src/lib.rs             |   7 ++
>   src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
>   src/api2/config/datastore.rs         |  22 +++-
>   src/api2/status.rs                   |  19 +++-
>   src/api2/types/mod.rs                |   2 +
>   src/backup/datastore.rs              |  23 ++++
>   src/backup/mod.rs                    |   2 +-
>   src/bin/proxmox-backup-api.rs        |  27 +++++
>   src/bin/proxmox-backup-manager.rs    |  41 +++++++
>   src/config/datastore.rs              |  16 +++
>   src/tools/disks/mod.rs               |  53 +++++++++
>   www/NavigationTree.js                |   3 +-
>   www/dashboard/DataStoreStatistics.js |   3 +
>   www/datastore/OptionView.js          |   6 +
>   www/datastore/Summary.js             |  77 ++++++++++++-
>   www/window/DataStoreEdit.js          |   5 +
>   17 files changed, 460 insertions(+), 8 deletions(-)
> 

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Thomas Lamprecht]

On 01.09.21 16:48, Dominik Csapak wrote:
> 
> also, would it not make sense to have
> multiple uuids per datastore instead of only one?
> 
> i guess most users of this will want to have more than
> one usb devices in rotation, and having multiple
> datastores + multiple sync jobs is probably
> a bit error prone. just allowing multiple
> uuids for each datastore would solve that
> problem nicely

Hmm, what would you do if more than one of them are plugged in at the same time?
Maybe hard to time on a running system, but possible if I connect a hub with
devices already plugged in, and can definitively happen (by mistake) on cold boot.

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dominik Csapak]

On 9/2/21 08:09, Thomas Lamprecht wrote:
> On 01.09.21 16:48, Dominik Csapak wrote:
>>
>> also, would it not make sense to have
>> multiple uuids per datastore instead of only one?
>>
>> i guess most users of this will want to have more than
>> one usb devices in rotation, and having multiple
>> datastores + multiple sync jobs is probably
>> a bit error prone. just allowing multiple
>> uuids for each datastore would solve that
>> problem nicely
> 
> Hmm, what would you do if more than one of them are plugged in at the same time?
> Maybe hard to time on a running system, but possible if I connect a hub with
> devices already plugged in, and can definitively happen (by mistake) on cold boot.
> 

my guess is that the udev rules do not trigger simultaneously ?
and even if, we could lock the mount/unmount so that this
does not make a problem and we simply mount the first
triggered one

but yeah, that was just an idea, not a hard requirement for me

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Thomas Lamprecht]

On 02.09.21 08:18, Dominik Csapak wrote:
> On 9/2/21 08:09, Thomas Lamprecht wrote:
>> On 01.09.21 16:48, Dominik Csapak wrote:
>>>
>>> also, would it not make sense to have
>>> multiple uuids per datastore instead of only one?
>>>
>>> i guess most users of this will want to have more than
>>> one usb devices in rotation, and having multiple
>>> datastores + multiple sync jobs is probably
>>> a bit error prone. just allowing multiple
>>> uuids for each datastore would solve that
>>> problem nicely
>>
>> Hmm, what would you do if more than one of them are plugged in at the same time?
>> Maybe hard to time on a running system, but possible if I connect a hub with
>> devices already plugged in, and can definitively happen (by mistake) on cold boot.
>>
> 
> my guess is that the udev rules do not trigger simultaneously ?

udev can coalesce events IIRC, and even if not it would not be guaranteed to
be stable so just relying on that fact would provide a bad UX IMO.

> and even if, we could lock the mount/unmount so that this
> does not make a problem and we simply mount the first
> triggered one

see above, IMO weird as the "first" is really not guaranteed to be
stable.

> but yeah, that was just an idea, not a hard requirement for me

IMO this is something we could always add later on without bending backwards,
if user really request it.

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Hannes Laimer]

Am 01.09.21 um 16:48 schrieb Dominik Csapak:
> High level comment here, rest in the relevant patches:
> 
> looks mostly ok, but one things stood out:
> 
> why do you add a command for the socket and the
> manager 'send command' method? would it not have
> been easier to call the mount api directly
> from the proxmox-backup-manager?
Since this is called by a udev rule it has to finish almost
immediately, calling the API(also through the pbs-manager)
takes to long, and everything forked will be killed. So
I had to somehow make the udev rule just 'dispatch' the mounting
command without actually doing it or starting another thread/process
to do it. The path to the socket file starts with a null byte and
is therefore not really accessible from outside of the rust code.
> 
> also, would it not make sense to have
> multiple uuids per datastore instead of only one?
> 
> i guess most users of this will want to have more than
> one usb devices in rotation, and having multiple
> datastores + multiple sync jobs is probably
> a bit error prone. just allowing multiple
> uuids for each datastore would solve that
> problem nicely
> 
> On 8/30/21 13:14, Hannes Laimer wrote:
>> v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
>>   - do not allow creation of removable datastores on a device that
>>     is mounted at '/'
>>   - ui: also show the mount point in the Options-tab
>>
>> Adds the possibility to create removable datastores. A removable
>> datastore has a UUID(the device on which the data is stored) and a
>> mount-point(where the device will be mounted), iff both are set the
>> datastore is removable. Everything else is identical to normal
>> datastores. Since config files for jobs, etc. are stored on the server,
>> all configuration can be done with the device plugged in or not. Certain
>> statistics about the datastore won't be available as long as it is not
>> plugged in.
>> Removable datastores have to be unmounted before removing, it can only
>> be unmounted if not jibs are running.
>> Removable datastores are mounted automatically when the device is
>> plugged in, if it has
>> been unmounted, it has to be mounted manually through the WebUI or the
>> Api.
>> Jobs will not be started if the datastore is not available, and
>> depending on the configuration, start when the device is plugged in the
>> next time.
>>
>> Still todo:
>>   - make sync to local datastore more integrated
>>   - (add 'when plugged in'-option to job schedule?)
>>   - replace linux commands with internal functions in tools/disks, where
>>      possible
>>
>> Hannes Laimer (15):
>>    tools: add disks utility functions
>>    config: add uuid+mountpoint to DataStoreConfig
>>    api2: add support for removable datastore creation
>>    backup: add check_if_available function to ds
>>    api2: add 'is_available' to DataStoreConfig
>>    api2: add 'removable' to DataStoreListItem
>>    api2: add (un)mount endpoint for removable ds's
>>    pbs: add mount-removable command to commandSocket
>>    pbs-manager: add 'send-command' command
>>    debian: add udev rule for removable datastores
>>    ui: show usb icon for removable datastore in list
>>    ui: add 'removable' checkbox in datastore creation
>>    ui: display row as disabled in ds statistics
>>    ui: show backing device UUID and mount-point in option tab
>>    ui: add (un)mount button to summary
>>
>>   debian/proxmox-backup-server.udev    |   3 +
>>   pbs-api-types/src/lib.rs             |   7 ++
>>   src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
>>   src/api2/config/datastore.rs         |  22 +++-
>>   src/api2/status.rs                   |  19 +++-
>>   src/api2/types/mod.rs                |   2 +
>>   src/backup/datastore.rs              |  23 ++++
>>   src/backup/mod.rs                    |   2 +-
>>   src/bin/proxmox-backup-api.rs        |  27 +++++
>>   src/bin/proxmox-backup-manager.rs    |  41 +++++++
>>   src/config/datastore.rs              |  16 +++
>>   src/tools/disks/mod.rs               |  53 +++++++++
>>   www/NavigationTree.js                |   3 +-
>>   www/dashboard/DataStoreStatistics.js |   3 +
>>   www/datastore/OptionView.js          |   6 +
>>   www/datastore/Summary.js             |  77 ++++++++++++-
>>   www/window/DataStoreEdit.js          |   5 +
>>   17 files changed, 460 insertions(+), 8 deletions(-)
>>
> 
>