public inbox for pmg-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Sterz <s.sterz@proxmox.com>
To: Daniel Tschlatscher <d.tschlatscher@proxmox.com>,
	pve-devel@lists.proxmox.com, pbs-devel@lists.proxmox.com,
	pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH http/common/manager/wt/backup/pmg v2] fix: #3971 Tasklog download button
Date: Thu, 8 Sep 2022 11:40:05 +0200	[thread overview]
Message-ID: <889eb91a-5795-fb74-ed17-4adc9a4d93a5@proxmox.com> (raw)
In-Reply-To: <20220907085633.89113-1-d.tschlatscher@proxmox.com>

On 9/7/22 10:56, Daniel Tschlatscher wrote:
> This patch series' aim is to add a download button in the tasklog-
> viewer GUI so that users may access all parts of the tasklog more
> easily (The tasklog-viewer only displays 50 lines at a time).
> 
> For this change in the GUI (proxmox-widget-toolkit) there were revised
> implementations for all backends (PVE, PMG and PBS) needed.
> Now, when the parameter 'limit' for the /log API call is set to '0',
> the backends will stream the file rather than returning a JSON array
> with the corresponding lines. (Beforehand, it would return 0 lines)
> 
> I initially included a number of patches aimed at refactoring
> redundant implementations of downloading a file via a button click.
> However, after talking with Sterzy, Dominik and Aaron a bit about
> it, I decided to only send the "core" task log download patch series
> now, because the refactoring depends on the changes made in widget-
> toolkit, but is done in the same repositories as the backend patches.
> This would make both reviewing and applying them more confusing, and
> the refactoring patches are "a step removed" anyway from this patch
> series. Therefore, I am going to send a separate patch series for them
> later instead.
> 
> 
> During the implementation of the file stream download I found a bug
> concerning chromium browsers because of which downloads "randomly"
> fail.
> The problem was, when implementing the download through an html a tag
> and setting its property "download" to anything, (even undefined)
> with a not imported self-signed certificate, the download will fail
> once the SSL connection is reset.
> In my testing this was the case after about 5 seconds. This problem is
> especially pronounced in the PMG as there are less background calls in
> contrast to the PVE. The PBS, however, does not seem to suffer this
> error at all, because the SSL handshake timeout seems to not run out.
>  
> Without the 'download' attribute you can't set the filename though.
> The file will only have a proper name if the server adds the correct
> 'content-disposition' header. A few calls to the 'downloadAsFile'
> function set the filename anyway, because in most cases the browser
> frontend is making enough polling calls so that the SSL handshake
> does not have to be executed for the download. (That is also why it
> probably did not come up before)
> This problem does not apply in Firefox as far as I could tell.
> 
> See https://bugs.chromium.org/p/chromium/issues/detail?id=993362 for
> more information on this.
> 
> -- Changes from v1 --
> * Rewrote commit messages and cover letter
> * Got rid of some refactoring which were initially included
>   (going to send as own patch series later)
> * Rebased to current repository versions
> 
> 
> Daniel Tschlatscher (1):
>   make tasklog downloadable in the backup server backend
> 
>  src/api2/node/tasks.rs             | 159 ++++++++++++++++++-----------
>  4 files changed, 107 insertions(+), 79 deletions(-)
> 
> Daniel Tschlatscher (1):
>   stream file method for PMG and PVE
> 
>  src/PVE/Tools.pm | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  
> Daniel Tschlatscher (1):
>   acknowledge content-disposition header
> 
>  src/PVE/APIServer/AnyEvent.pm | 3 +++
>  1 file changed, 3 insertions(+)
>  
> Daniel Tschlatscher (1):
>   Revised task log API call when parameter 'limit' is 0
> 
>  PVE/API2/Tasks.pm                 | 13 ++++++++++---
>  3 files changed, 15 insertions(+), 24 deletions(-)
>  
> Daniel Tschlatscher (1):
>   revised task log download function in the PMG backend
> 
>  src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
>  1 file changed, 11 insertions(+), 23 deletions(-)
> 
> Daniel Tschlatscher (3):
>   Source file download call in central function
>   add task log download button in TaskViewer
> 
>  src/Utils.js              | 13 +++++++++++++
>  src/window/TaskViewer.js  | 17 +++++++++++++++--
>  src/window/FileBrowser.js | 11 +++++------
>  3 files changed, 38 insertions(+), 8 deletions(-)
> 

Other than one bug in `pmg-api` (see my other email) this series seems
to work as intended. The bug in `pmg-api` is fixed fairly easily by just
calling the right function and I tested that locally already. So unless
there are other considerations:

Tested-by: Stefan Sterz <s.sterz@proxmox.com>




      parent reply	other threads:[~2022-09-08  9:40 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-07  8:56 Daniel Tschlatscher
2022-09-07  8:56 ` [pmg-devel] [PATCH proxmox-backup v2 1/7] make tasklog downloadable in the backup server backend Daniel Tschlatscher
2022-09-07  8:56 ` [pmg-devel] [PATCH http-server v2 2/7] acknowledge content-disposition header Daniel Tschlatscher
2022-09-29 12:36   ` [pmg-devel] applied: [pve-devel] " Thomas Lamprecht
2022-09-07  8:56 ` [pmg-devel] [PATCH common v2 3/7] stream file method for PMG and PVE Daniel Tschlatscher
2022-09-07  8:56 ` [pmg-devel] [PATCH manager v2 4/7] revised task log API call for PVE Daniel Tschlatscher
2022-10-05 12:30   ` [pmg-devel] [pve-devel] " Thomas Lamprecht
2022-10-10 11:40     ` Daniel Tschlatscher
2022-10-10 13:10       ` Thomas Lamprecht
2022-09-07  8:56 ` [pmg-devel] [PATCH pmg-api v2 5/7] revised task log download function in the PMG backend Daniel Tschlatscher
2022-09-08  9:36   ` Stefan Sterz
2022-09-08 11:19     ` Daniel Tschlatscher
2022-09-07  8:56 ` [pmg-devel] [PATCH widget-toolkit v2 6/7] Source file download call in central function Daniel Tschlatscher
2022-09-07  8:56 ` [pmg-devel] [PATCH widget-toolkit v2 7/7] add task log download button in TaskViewer Daniel Tschlatscher
2022-09-08  9:40 ` Stefan Sterz [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=889eb91a-5795-fb74-ed17-4adc9a4d93a5@proxmox.com \
    --to=s.sterz@proxmox.com \
    --cc=d.tschlatscher@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=pmg-devel@lists.proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal