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

Cancel downloadRequest when timeout without downloadURL #5329

Conversation

kaovilai
Copy link
Contributor

@kaovilai kaovilai commented Sep 12, 2022

Tested with go install ./cmd/velero

Result takes about one minute to fail log command for a backup that no longer have backup storage location in the cluster. One minute default came from

timeout := time.Minute

❯ time velero backup logs mongo-csi-e2e-61f370ce-3056-11ed-a685-1acc26a6847e --timeout 5s
I0914 11:52:41.073495   80568 request.go:665] Waited for 1.063170625s due to client-side throttling, not priority and fairness, request: GET:https://api.cluster-tkaovila-apr23-410.tkaovila-apr23-410.mg.dog8code.com:6443/apis/samples.operator.openshift.io/v1?timeout=32s
An error occurred: download request download url timeout, check velero server logs for errors. backup storage location may not be available
velero backup logs mongo-csi-e2e-61f370ce-3056-11ed-a685-1acc26a6847e  5s  0.10s user 0.06s system 1% cpu 10.193 total

Thank you for contributing to Velero!

Please add a summary of your change

Cancel download request if timeout waiting for download URL

Does your change fix a particular issue?

Fixes #5324

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #5329 (fea670b) into main (be40d7e) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head fea670b differs from pull request most recent head 045ba52. Consider uploading reports for the commit 045ba52 to get more accurate results

@@            Coverage Diff             @@
##             main    #5329      +/-   ##
==========================================
- Coverage   40.88%   40.82%   -0.07%     
==========================================
  Files         233      234       +1     
  Lines       20241    20260      +19     
==========================================
- Hits         8276     8271       -5     
- Misses      11364    11388      +24     
  Partials      601      601              
Impacted Files Coverage Δ
pkg/uploader/provider/kopia.go 52.84% <0.00%> (-4.07%) ⬇️
pkg/podvolume/backupper.go 4.66% <0.00%> (-0.15%) ⬇️
pkg/cmd/server/server.go 6.59% <0.00%> (-0.02%) ⬇️
pkg/podvolume/backupper_factory.go 0.00% <0.00%> (ø)
pkg/util/kube/pod.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@reasonerjt
Copy link
Contributor

This code change LGTM
I don't quite understand why the Test TestGetVolumesRepositoryType failed.
@Lyndon-Li any idea?

@kaovilai kaovilai changed the title Cancel downloadRequest if timeout passed waiting for downloadURL Cancel downloadRequest when timeout without downloadURL Sep 14, 2022
@kaovilai kaovilai force-pushed the veleroDownloadRequestReturnSomething-veleromain branch 3 times, most recently from e6821bc to 045ba52 Compare September 14, 2022 15:32
@kaovilai
Copy link
Contributor Author

Updated PR description with test results.

@kaovilai
Copy link
Contributor Author

@reasonerjt all green now :D

@kaovilai kaovilai force-pushed the veleroDownloadRequestReturnSomething-veleromain branch from 045ba52 to a7d702c Compare September 14, 2022 15:59
@kaovilai
Copy link
Contributor Author

added error "download request download url timeout, check velero server logs for errors. backup storage location may not be available"

Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @kaovilai !

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai
Copy link
Contributor Author

An alternative implementation that we could do for newer/main would be to add error field to downloadRequestStatus and add a new phase error.

@Lyndon-Li Lyndon-Li merged commit 429e204 into vmware-tanzu:main Sep 19, 2022
@kaovilai kaovilai deleted the veleroDownloadRequestReturnSomething-veleromain branch November 21, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

velero CLI can become stuck waiting for downloadrequest.status.downloadURL != ""
6 participants