make kubectl cp resume on transfer error#104792
Conversation
34df723 to
d8b5e68
Compare
There was a problem hiding this comment.
I don't know how related is to this, but Ben and I had to spend a lot of time to make it work flawlessly, to stream a tar over a network pipe, check if you can find something useful for this https://github.com/kubernetes-sigs/kind/blob/b6bc112522651d98c81823df56b7afa511459a3b/pkg/cluster/internal/logs/logs.go#L39-L60
There was a problem hiding this comment.
Yeah nice trick the shell invocation to have access to pipes...
I'm using tail to skip the first n bytes of the archive when resuming, however the best one is the pattern where I use a reader of a reader to catch errors and keep track of the transferred data to reconnect without untarAll noticing.
There was a problem hiding this comment.
that is only valid if the source has not changed during the re connection, right?
There was a problem hiding this comment.
yup, in this respect I don't improve on current implementation - however if your use case is to retrieve GBs of backups, usability is much higher
I might add a flag to set a max retry, but for the moment I'm waiting for some feedback on the issue with a prebuilt binary
There was a problem hiding this comment.
For backwards compatibility and when not using retries (the proposed default, above) please use only tar, since tail isn't required in that case.
|
/assign @deads2k |
|
Hey @matthyx. Retrying seems to work perfectly, though it fails after fetching 2362411520 bytes with error |
|
/assign @soltysh |
soltysh
left a comment
There was a problem hiding this comment.
This looks mostly good, please address the bits I've mentioned and ping me on slack to get it included in 1.23.
There was a problem hiding this comment.
I'd prefer this to be opt-in for starters, so default to be 0, and don't use -t as the shorthand, since that's being used elsewhere as shorthand for --tty so that might be confusing here a bit.
There was a problem hiding this comment.
For backwards compatibility and when not using retries (the proposed default, above) please use only tar, since tail isn't required in that case.
|
/triage accepted |
Thanks @soltysh updated the PR accordingly. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matthyx, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allows
kubectl cptransfers to continue despite network errors.Which issue(s) this PR fixes:
Fixes #60140
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: