Disable SPDY pings for kubectl cp and add --ping flag for kubectl exec

[brianpursley]

What type of PR is this?

/kind bug

What this PR does / why we need it:

SPDY pings seem to cause sporadic incomplete file transfer for kubectl cp and kubectl exec.

This PR disables pings for kubectl cp and makes them configurable via a --ping flag for kubectl exec.

Which issue(s) this PR fixes:

Fixes #60140

Special notes for your reviewer:

Further context: #60140 (comment)

Does this PR introduce a user-facing change?

Fixed a bug where using kubectl cp to copy a large file from a remote pod would sometimes result in an incomplete file.
Added --ping flag to allow ping frequency to be configured or disabled for kubectl exec.

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

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[brianpursley]

@dims How is this to start with?

I tried to leave the default behavior or spdy.RoundTripperFor() and remotecommand.NewSPDYExecutor() intact, in case someone other than kubectl is depending on that, so I added a couple of new functions for specifying a custom ping period.

[brianpursley]

What I don't like is that NewSPDYExecutorWithPings() implies that NewSPDYExecutor() doesn't have pings. But that is not the case. NewSPDYExecutor has the default ping period (5 seconds).

Same with RoundTripperWithPingsFor() and RoundTripperFor(). The latter uses the default ping period.

The comments mention the distinction, but the naming seems a little off.

Any recommendations on better naming for these?

[brianpursley]

I can add/update tests, but wanted to see what people think about this change first.

[dims]

@brianpursley naming is hard :) ( https://martinfowler.com/bliki/TwoHardThings.html ) let's see if anyone else comes up with other ideas

[aojea]

This also solves

cri-o/cri-o#4995
containerd/containerd#5563
#102569

cc: @haircommander @mikebrow

@mikedanese has also some SPDY changes in flight
@soltysh for cli and UX

[brianpursley]

Added some tests

[cici37]

/remove-sig api-machinery

[brianpursley]

Rebased. Updated help text and log message.

[cici37]

/remove-sig api-machinery

[k8s-ci-robot]

@cici37: Those labels are not set on the issue: sig/api-machinery

In response to this:

/remove-sig api-machinery

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[brianpursley]

/hold

@soltysh any thoughts on this PR? It adds a flag, which I know we like to avoid, but it fixes a known problem with kubectl cp.

Also a factor, I know @seans3 is doing some work to switch from spdy to websockets, so I'm not sure if the timeline for that might affect the decision to fix this problem or just wait and see if websockets will fix it.

[shnkr]

@soltysh any thoughts on this PR? It adds a flag, which I know we like to avoid, but it fixes a known problem with kubectl cp.

As someone who had been facing this issue for over a couple of years on off, disabling/enabling/changing interval of pings didn't help much. The latency was one part of the problem which I was able to mitigate but the other part was with various intermediaries. Once in a blue moon, I get to deal with it and every time it's a new environment with a new customer and takes away so much time.

Also a factor, I know @seans3 is doing some work to switch from spdy to websockets, so I'm not sure if the timeline for that might affect the decision to fix this problem or just wait and see if websockets will fix it.

I doubt fix #116778 will solve it completely but time will tell. Otherwise I've been using client<->kubelet leg on websocket without changing kubelet<->runtime leg and it didn't make any difference.

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Joseph-Goergen]

/reopen

[k8s-ci-robot]

@Joseph-Goergen: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mikebrow]

/reopen

[k8s-ci-robot]

@mikebrow: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brianpursley
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apimachinery/pkg/util/OWNERS
• staging/src/k8s.io/client-go/OWNERS
• staging/src/k8s.io/kubectl/OWNERS [brianpursley]

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

[leilajal]

/remove-sig api-machinery

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Joseph-Goergen]

/reopen

[k8s-ci-robot]

@Joseph-Goergen: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.