Skip to content

make kubectl cp resume on transfer error#104792

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
matthyx:60140
Nov 10, 2021
Merged

make kubectl cp resume on transfer error#104792
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
matthyx:60140

Conversation

@matthyx

@matthyx matthyx commented Sep 6, 2021

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Allows kubectl cp transfers 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?

adding option for kubectl cp to resume on network errors until completion, requires tar in addition to tail inside the container image

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 6, 2021
@matthyx matthyx marked this pull request as draft September 6, 2021 08:12
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2021
@matthyx matthyx force-pushed the 60140 branch 2 times, most recently from 34df723 to d8b5e68 Compare September 6, 2021 08:16

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@matthyx matthyx Sep 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that is only valid if the source has not changed during the re connection, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For backwards compatibility and when not using retries (the proposed default, above) please use only tar, since tail isn't required in that case.

@matthyx matthyx marked this pull request as ready for review September 17, 2021 07:33
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2021
@matthyx matthyx changed the title make kubectl cp resume infinitely on transfer error make kubectl cp resume on transfer error Sep 17, 2021
@matthyx

matthyx commented Sep 19, 2021

Copy link
Copy Markdown
Contributor Author

/assign @deads2k

@m-makuch

Copy link
Copy Markdown

Hey @matthyx. Retrying seems to work perfectly, though it fails after fetching 2362411520 bytes with error tail: write error: Bad address. Tried few times, each time seems to fail after 2.4GB download.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021
@matthyx

matthyx commented Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

/assign @soltysh

@soltysh soltysh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks mostly good, please address the bits I've mentioned and ping me on slack to get it included in 1.23.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For backwards compatibility and when not using retries (the proposed default, above) please use only tar, since tail isn't required in that case.

@soltysh

soltysh commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 8, 2021
@matthyx

matthyx commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author

This looks mostly good, please address the bits I've mentioned and ping me on slack to get it included in 1.23.

Thanks @soltysh updated the PR accordingly.

@soltysh soltysh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8237943 into kubernetes:master Nov 10, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Nov 10, 2021
@matthyx matthyx deleted the 60140 branch November 11, 2021 06:41
QuasarQuirkMaster

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kubectl cp fails on large files

8 participants