* [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
@ 2021-06-10 7:37 Dominik Csapak
2021-06-10 8:23 ` Stefan Reiter
0 siblings, 1 reply; 7+ messages in thread
From: Dominik Csapak @ 2021-06-10 7:37 UTC (permalink / raw)
To: pve-devel
and not the node where the browser connects.
there are at least two good reasons for this:
* it is confusing, since the user would expect it to start where
the ui is pointint to
* the storage may not be available on the node the browser connects
to, but it must be available on the node selected in the ui
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/grid/BackupView.js | 4 ++--
www/manager6/storage/BackupView.js | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index 8825ed96..fbed4118 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -247,8 +247,8 @@ Ext.define('PVE.grid.BackupView', {
let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
Ext.create('Proxmox.window.FileBrowser', {
title: gettext('File Restore') + " - " + rec.data.text,
- listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
- downloadURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
+ listURL: `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/list`,
+ downloadURL: `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/download`,
extraParams: {
volume: rec.data.volid,
},
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 0613c94d..c287ec63 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -114,8 +114,8 @@ Ext.define('PVE.storage.BackupView', {
let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
Ext.create('Proxmox.window.FileBrowser', {
title: gettext('File Restore') + " - " + rec.data.text,
- listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
- downloadURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
+ listURL: `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/list`,
+ downloadURL: `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/download`,
extraParams: {
volume: rec.data.volid,
},
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-10 7:37 [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node Dominik Csapak
@ 2021-06-10 8:23 ` Stefan Reiter
2021-06-10 8:40 ` Dominik Csapak
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Reiter @ 2021-06-10 8:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
On 6/10/21 9:37 AM, Dominik Csapak wrote:
> and not the node where the browser connects.
> there are at least two good reasons for this:
> * it is confusing, since the user would expect it to start where
> the ui is pointint to
> * the storage may not be available on the node the browser connects
> to, but it must be available on the node selected in the ui
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
Does this work with the proxying code for downloading files? I believe
my original reasoning behind this was because my implementation for
quickly forwarding data between the unprivileged and privileged daemons
uses a local Unix socket. Thus forwarding between nodes (as I understand
this will do?) would be subject to caching the data at the node being
contacted, before sending it to the browser.
> www/manager6/grid/BackupView.js | 4 ++--
> www/manager6/storage/BackupView.js | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
> index 8825ed96..fbed4118 100644
> --- a/www/manager6/grid/BackupView.js
> +++ b/www/manager6/grid/BackupView.js
> @@ -247,8 +247,8 @@ Ext.define('PVE.grid.BackupView', {
> let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
> Ext.create('Proxmox.window.FileBrowser', {
> title: gettext('File Restore') + " - " + rec.data.text,
> - listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
> - downloadURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
> + listURL: `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/list`,
> + downloadURL: `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/download`,
> extraParams: {
> volume: rec.data.volid,
> },
> diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
> index 0613c94d..c287ec63 100644
> --- a/www/manager6/storage/BackupView.js
> +++ b/www/manager6/storage/BackupView.js
> @@ -114,8 +114,8 @@ Ext.define('PVE.storage.BackupView', {
> let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
> Ext.create('Proxmox.window.FileBrowser', {
> title: gettext('File Restore') + " - " + rec.data.text,
> - listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
> - downloadURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
> + listURL: `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/list`,
> + downloadURL: `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/download`,
> extraParams: {
> volume: rec.data.volid,
> },
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-10 8:23 ` Stefan Reiter
@ 2021-06-10 8:40 ` Dominik Csapak
2021-06-10 9:29 ` Stefan Reiter
0 siblings, 1 reply; 7+ messages in thread
From: Dominik Csapak @ 2021-06-10 8:40 UTC (permalink / raw)
To: Stefan Reiter, Proxmox VE development discussion
On 6/10/21 10:23, Stefan Reiter wrote:
> On 6/10/21 9:37 AM, Dominik Csapak wrote:
>> and not the node where the browser connects.
>> there are at least two good reasons for this:
>> * it is confusing, since the user would expect it to start where
>> the ui is pointint to
>> * the storage may not be available on the node the browser connects
>> to, but it must be available on the node selected in the ui
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>
> Does this work with the proxying code for downloading files? I believe
> my original reasoning behind this was because my implementation for
> quickly forwarding data between the unprivileged and privileged daemons
> uses a local Unix socket. Thus forwarding between nodes (as I understand
> this will do?) would be subject to caching the data at the node being
> contacted, before sending it to the browser.
well it does work, since i sucessfully downloaded some files from a backup
what exactly do you mean with
> would be subject to caching the data at the node being
> contacted, before sending it to the browser.
yes the download api call goes afaiu
proxy@node1(the one the browser connects to) ->
proxy@node2 (node selected from tree) ->
daemon@node2 ->
vm@node2
>
>> www/manager6/grid/BackupView.js | 4 ++--
>> www/manager6/storage/BackupView.js | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/www/manager6/grid/BackupView.js
>> b/www/manager6/grid/BackupView.js
>> index 8825ed96..fbed4118 100644
>> --- a/www/manager6/grid/BackupView.js
>> +++ b/www/manager6/grid/BackupView.js
>> @@ -247,8 +247,8 @@ Ext.define('PVE.grid.BackupView', {
>> let isVMArchive =
>> PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
>> Ext.create('Proxmox.window.FileBrowser', {
>> title: gettext('File Restore') + " - " + rec.data.text,
>> - listURL:
>> `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
>> - downloadURL:
>> `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
>> + listURL:
>> `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/list`,
>> + downloadURL:
>> `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/download`,
>> extraParams: {
>> volume: rec.data.volid,
>> },
>> diff --git a/www/manager6/storage/BackupView.js
>> b/www/manager6/storage/BackupView.js
>> index 0613c94d..c287ec63 100644
>> --- a/www/manager6/storage/BackupView.js
>> +++ b/www/manager6/storage/BackupView.js
>> @@ -114,8 +114,8 @@ Ext.define('PVE.storage.BackupView', {
>> let isVMArchive =
>> PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
>> Ext.create('Proxmox.window.FileBrowser', {
>> title: gettext('File Restore') + " - " + rec.data.text,
>> - listURL:
>> `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
>> - downloadURL:
>> `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
>> + listURL:
>> `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/list`,
>> + downloadURL:
>> `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/download`,
>>
>> extraParams: {
>> volume: rec.data.volid,
>> },
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-10 8:40 ` Dominik Csapak
@ 2021-06-10 9:29 ` Stefan Reiter
2021-06-10 11:28 ` Dominik Csapak
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Reiter @ 2021-06-10 9:29 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On 6/10/21 10:40 AM, Dominik Csapak wrote:
> On 6/10/21 10:23, Stefan Reiter wrote:
>> On 6/10/21 9:37 AM, Dominik Csapak wrote:
>>> and not the node where the browser connects.
>>> there are at least two good reasons for this:
>>> * it is confusing, since the user would expect it to start where
>>> the ui is pointint to
>>> * the storage may not be available on the node the browser connects
>>> to, but it must be available on the node selected in the ui
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>
>> Does this work with the proxying code for downloading files? I believe
>> my original reasoning behind this was because my implementation for
>> quickly forwarding data between the unprivileged and privileged
>> daemons uses a local Unix socket. Thus forwarding between nodes (as I
>> understand this will do?) would be subject to caching the data at the
>> node being contacted, before sending it to the browser.
>
> well it does work, since i sucessfully downloaded some files from a backup
>
> what exactly do you mean with
> > would be subject to caching the data at the node being
> > contacted, before sending it to the browser.
>
Try downloading a larger file (multiple GB) - is it streamed directly to
the browser or first transferred to node1 (from your example below),
cached in memory there, and only once it's fully in RAM transferred to
the client? I believe with this patch, the second variant will happen.
This whole idea was the reason for these shenanigans:
https://git.proxmox.com/?p=pve-http-server.git;a=commitdiff;h=51841e98fa5d4ad4d5b5250523c45f88769c577f
> yes the download api call goes afaiu
>
> proxy@node1(the one the browser connects to) ->
> proxy@node2 (node selected from tree) ->
> daemon@node2 ->
> vm@node2
>
>>
>>> www/manager6/grid/BackupView.js | 4 ++--
>>> www/manager6/storage/BackupView.js | 4 ++--
>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/www/manager6/grid/BackupView.js
>>> b/www/manager6/grid/BackupView.js
>>> index 8825ed96..fbed4118 100644
>>> --- a/www/manager6/grid/BackupView.js
>>> +++ b/www/manager6/grid/BackupView.js
>>> @@ -247,8 +247,8 @@ Ext.define('PVE.grid.BackupView', {
>>> let isVMArchive =
>>> PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
>>> Ext.create('Proxmox.window.FileBrowser', {
>>> title: gettext('File Restore') + " - " + rec.data.text,
>>> - listURL:
>>> `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
>>> - downloadURL:
>>> `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
>>> + listURL:
>>> `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/list`,
>>> + downloadURL:
>>> `/api2/json/nodes/${nodename}/storage/${storage}/file-restore/download`,
>>> extraParams: {
>>> volume: rec.data.volid,
>>> },
>>> diff --git a/www/manager6/storage/BackupView.js
>>> b/www/manager6/storage/BackupView.js
>>> index 0613c94d..c287ec63 100644
>>> --- a/www/manager6/storage/BackupView.js
>>> +++ b/www/manager6/storage/BackupView.js
>>> @@ -114,8 +114,8 @@ Ext.define('PVE.storage.BackupView', {
>>> let isVMArchive =
>>> PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
>>> Ext.create('Proxmox.window.FileBrowser', {
>>> title: gettext('File Restore') + " - " + rec.data.text,
>>> - listURL:
>>> `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
>>> - downloadURL:
>>> `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
>>>
>>> + listURL:
>>> `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/list`,
>>> + downloadURL:
>>> `/api2/json/nodes/${nodename}/storage/${me.storage}/file-restore/download`,
>>>
>>> extraParams: {
>>> volume: rec.data.volid,
>>> },
>>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-10 9:29 ` Stefan Reiter
@ 2021-06-10 11:28 ` Dominik Csapak
2021-06-18 15:17 ` Thomas Lamprecht
0 siblings, 1 reply; 7+ messages in thread
From: Dominik Csapak @ 2021-06-10 11:28 UTC (permalink / raw)
To: Stefan Reiter, Proxmox VE development discussion
On 6/10/21 11:29, Stefan Reiter wrote:
> On 6/10/21 10:40 AM, Dominik Csapak wrote:
>> On 6/10/21 10:23, Stefan Reiter wrote:
>>> On 6/10/21 9:37 AM, Dominik Csapak wrote:
>>>> and not the node where the browser connects.
>>>> there are at least two good reasons for this:
>>>> * it is confusing, since the user would expect it to start where
>>>> the ui is pointint to
>>>> * the storage may not be available on the node the browser connects
>>>> to, but it must be available on the node selected in the ui
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>
>>> Does this work with the proxying code for downloading files? I
>>> believe my original reasoning behind this was because my
>>> implementation for quickly forwarding data between the unprivileged
>>> and privileged daemons uses a local Unix socket. Thus forwarding
>>> between nodes (as I understand this will do?) would be subject to
>>> caching the data at the node being contacted, before sending it to
>>> the browser.
>>
>> well it does work, since i sucessfully downloaded some files from a
>> backup
>>
>> what exactly do you mean with
>> > would be subject to caching the data at the node being
>> > contacted, before sending it to the browser.
>>
>
> Try downloading a larger file (multiple GB) - is it streamed directly to
> the browser or first transferred to node1 (from your example below),
> cached in memory there, and only once it's fully in RAM transferred to
> the client? I believe with this patch, the second variant will happen.
>
> This whole idea was the reason for these shenanigans:
> https://git.proxmox.com/?p=pve-http-server.git;a=commitdiff;h=51841e98fa5d4ad4d5b5250523c45f88769c577f
>
>
from a short test it seem you're right
(have a pveproxy worker that sits at 2GiB RES atm)
but the solution here is probably to drop the 'proxyto'
in the api call, and do the same as we do for
vnc proxying? ssh tunnel and starting the file
restore on the target node
>> yes the download api call goes afaiu
>>
>> proxy@node1(the one the browser connects to) ->
>> proxy@node2 (node selected from tree) ->
>> daemon@node2 ->
>> vm@node2
>>
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-10 11:28 ` Dominik Csapak
@ 2021-06-18 15:17 ` Thomas Lamprecht
2021-06-21 9:38 ` Stefan Reiter
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Lamprecht @ 2021-06-18 15:17 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Stefan Reiter
On 10.06.21 13:28, Dominik Csapak wrote:
> On 6/10/21 11:29, Stefan Reiter wrote:
>> On 6/10/21 10:40 AM, Dominik Csapak wrote:
>>> On 6/10/21 10:23, Stefan Reiter wrote:
>>>> On 6/10/21 9:37 AM, Dominik Csapak wrote:
>>>>> and not the node where the browser connects.
>>>>> there are at least two good reasons for this:
>>>>> * it is confusing, since the user would expect it to start where
>>>>> the ui is pointint to
>>>>> * the storage may not be available on the node the browser connects
>>>>> to, but it must be available on the node selected in the ui
>>>>>
>>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>>> ---
>>>>
>>>> Does this work with the proxying code for downloading files? I believe my original reasoning behind this was because my implementation for quickly forwarding data between the unprivileged and privileged daemons uses a local Unix socket. Thus forwarding between nodes (as I understand this will do?) would be subject to caching the data at the node being contacted, before sending it to the browser.
>>>
>>> well it does work, since i sucessfully downloaded some files from a backup
>>>
>>> what exactly do you mean with
>>> > would be subject to caching the data at the node being
>>> > contacted, before sending it to the browser.
>>>
>>
>> Try downloading a larger file (multiple GB) - is it streamed directly to the browser or first transferred to node1 (from your example below), cached in memory there, and only once it's fully in RAM transferred to the client? I believe with this patch, the second variant will happen.
>>
>> This whole idea was the reason for these shenanigans:
>> https://git.proxmox.com/?p=pve-http-server.git;a=commitdiff;h=51841e98fa5d4ad4d5b5250523c45f88769c577f
>>
>
> from a short test it seem you're right
> (have a pveproxy worker that sits at 2GiB RES atm)
>
> but the solution here is probably to drop the 'proxyto'
> in the api call, and do the same as we do for
> vnc proxying? ssh tunnel and starting the file
> restore on the target node
@Stefan Any feedback regarding that proposal (or alternative fix)?
I mean, in the gui it may still make sense to use the local node if the storage is
available there, as it normally means less total traffic produced, but if it's not
configured as available there that's just not an option.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node
2021-06-18 15:17 ` Thomas Lamprecht
@ 2021-06-21 9:38 ` Stefan Reiter
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Reiter @ 2021-06-21 9:38 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak
On 6/18/21 5:17 PM, Thomas Lamprecht wrote:
> On 10.06.21 13:28, Dominik Csapak wrote:
>> On 6/10/21 11:29, Stefan Reiter wrote:
>>> On 6/10/21 10:40 AM, Dominik Csapak wrote:
>>>> On 6/10/21 10:23, Stefan Reiter wrote:
>>>>> On 6/10/21 9:37 AM, Dominik Csapak wrote:
>>>>>> and not the node where the browser connects.
>>>>>> there are at least two good reasons for this:
>>>>>> * it is confusing, since the user would expect it to start where
>>>>>> the ui is pointint to
>>>>>> * the storage may not be available on the node the browser connects
>>>>>> to, but it must be available on the node selected in the ui
>>>>>>
>>>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>>>> ---
>>>>>
>>>>> Does this work with the proxying code for downloading files? I believe my original reasoning behind this was because my implementation for quickly forwarding data between the unprivileged and privileged daemons uses a local Unix socket. Thus forwarding between nodes (as I understand this will do?) would be subject to caching the data at the node being contacted, before sending it to the browser.
>>>>
>>>> well it does work, since i sucessfully downloaded some files from a backup
>>>>
>>>> what exactly do you mean with
>>>> > would be subject to caching the data at the node being
>>>> > contacted, before sending it to the browser.
>>>>
>>>
>>> Try downloading a larger file (multiple GB) - is it streamed directly to the browser or first transferred to node1 (from your example below), cached in memory there, and only once it's fully in RAM transferred to the client? I believe with this patch, the second variant will happen.
>>>
>>> This whole idea was the reason for these shenanigans:
>>> https://git.proxmox.com/?p=pve-http-server.git;a=commitdiff;h=51841e98fa5d4ad4d5b5250523c45f88769c577f
>>>
>>
>> from a short test it seem you're right
>> (have a pveproxy worker that sits at 2GiB RES atm)
>>
>> but the solution here is probably to drop the 'proxyto'
>> in the api call, and do the same as we do for
>> vnc proxying? ssh tunnel and starting the file
>> restore on the target node
>
> @Stefan Any feedback regarding that proposal (or alternative fix)?
>
> I mean, in the gui it may still make sense to use the local node if the storage is
> available there, as it normally means less total traffic produced, but if it's not
> configured as available there that's just not an option.
>
I talked with Dominik and we agreed that his idea is the right one. We left it at
"one of us will implement it", since we deemed it not that big of an issue for right
now, but I can take a look sooner as well if needed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-21 9:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 7:37 [pve-devel] [PATCH manager] ui: file-restore: start the file-restore on the selected node Dominik Csapak
2021-06-10 8:23 ` Stefan Reiter
2021-06-10 8:40 ` Dominik Csapak
2021-06-10 9:29 ` Stefan Reiter
2021-06-10 11:28 ` Dominik Csapak
2021-06-18 15:17 ` Thomas Lamprecht
2021-06-21 9:38 ` Stefan Reiter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox