Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace grab with internal download function #5020

Merged
merged 8 commits into from
Sep 27, 2024

Conversation

twz123
Copy link
Member

@twz123 twz123 commented Sep 20, 2024

Description

This makes the download process use a temporary file and atomic file replacements on success. Allows existing files to be re-downloaded, which was not possible before. On the contrary, it will re-download files even if they didn't change. Add a very long overall timeout for Autopilot downloads. This will ensure that the download will fail at some point, even if the remote server is artificially slow. Remove logger and httpResponse fields from the downloader struct.

Introduce internal/http.Download: A simple HTTP download function that can be used to download stuff from the Internet. It will detect and abort stale downloads as soon as the data transfer stalls for a certain amount of time. Special care is taken to detect and sanitize server-suggested file names.

Add k0scontext.WithInactivityTimeout, which returns a context that times out after a specified period of inactivity. It provides a "keep alive" mechanism to reset the timeout based on recent activity, ensuring that the context will remain valid for as long as there is activity. This is used to implement the cancellation of stale downloads.

Fixes:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@twz123 twz123 added bug Something isn't working component/autopilot labels Sep 20, 2024
@twz123 twz123 force-pushed the http-downloader branch 5 times, most recently from 9af0df8 to d60a190 Compare September 20, 2024 15:34
@twz123 twz123 marked this pull request as ready for review September 20, 2024 18:13
@twz123 twz123 requested a review from a team as a code owner September 20, 2024 18:13
@twz123 twz123 requested review from kke and jnummelin September 20, 2024 18:13
@twz123 twz123 force-pushed the http-downloader branch 5 times, most recently from 2b88f4c to 461e29e Compare September 23, 2024 07:12
@twz123
Copy link
Member Author

twz123 commented Sep 23, 2024

Maybe we need #5024 first to get the CI green here.

@jnummelin jnummelin added this to the 1.31 milestone Sep 23, 2024
go.mod Show resolved Hide resolved
pkg/autopilot/download/downloader.go Outdated Show resolved Hide resolved
jnummelin
jnummelin previously approved these changes Sep 25, 2024
Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

This is a simple HTTP download function that can be used to download
stuff from the Internet. Special care is taken to detect and sanitize
server-suggested file names. Intended to replace the dependency on grab.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
It may very well be a local variable instead.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
This makes the download process use a temporary file and atomic file
replacements on success. Allows existing files to be re-downloaded,
which was not possible before. On the contrary, it will re-download files
even if they didn't change.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Returns a context that times out after a specified period of inactivity.
It provides a "keep alive" mechanism to reset the timeout based on
recent activity, ensuring that the context will remain valid for as long
as there is activity.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Moves the private implementation from the integration tests into the
regular code base. This adapter is useful from time to time.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Cancel downloads whenever there's no data flowing for a certain amount
of time. Use k0scontext.WithInactivityTimeout for that.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
This will ensure that the download will fail at some point, even if the
remote server is artificially slow.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
@twz123 twz123 merged commit e8ffabd into k0sproject:main Sep 27, 2024
87 checks passed
@twz123 twz123 deleted the http-downloader branch September 27, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/autopilot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants