Disable SPDY pings for kubectl cp and add --ping flag for kubectl exec by brianpursley · Pull Request #115493 · kubernetes/kubernetes

brianpursley · 2023-02-03T02:06:21Z

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 · 2023-02-03T02:06:30Z

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.

Details

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 · 2023-02-03T02:10:14Z

@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 · 2023-02-03T02:20:46Z

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 · 2023-02-03T02:28:00Z

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

dims · 2023-02-03T03:42:02Z

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

aojea · 2023-02-04T11:29:42Z

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 · 2023-02-12T23:25:44Z

Added some tests

cici37 · 2023-02-14T21:09:06Z

/remove-sig api-machinery

brianpursley · 2023-05-23T20:51:48Z

Rebased. Updated help text and log message.

cici37 · 2023-05-30T20:02:44Z

/remove-sig api-machinery

k8s-ci-robot · 2023-05-30T20:02:57Z

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

Details

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 · 2023-05-30T20:10:33Z

/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 · 2023-08-14T20:40:31Z

@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 · 2023-09-09T01:14:35Z

PR needs rebase.

Details

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 · 2024-02-04T04:14:28Z

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 · 2024-02-04T04:14:33Z

@k8s-triage-robot: Closed this PR.

Details

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 · 2024-02-21T20:25:55Z

/reopen

k8s-ci-robot · 2024-02-21T20:25:59Z

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

Details

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 · 2024-02-28T21:07:30Z

/reopen

k8s-ci-robot · 2024-02-28T21:07:35Z

@mikebrow: Reopened this PR.

Details

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 · 2024-02-28T21:08:10Z

[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.

Details

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 · 2024-03-26T19:36:12Z

/remove-sig api-machinery

k8s-triage-robot · 2024-04-25T20:15:45Z

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 · 2024-04-25T20:15:49Z

@k8s-triage-robot: Closed this PR.

Details

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 · 2024-04-29T13:40:56Z

/reopen

k8s-ci-robot · 2024-04-29T13:40:59Z

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

Details

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 added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 3, 2023

k8s-ci-robot added the area/kubectl label Feb 3, 2023

k8s-ci-robot requested review from aojea and ardaguclu February 3, 2023 02:10

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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 Feb 3, 2023

brianpursley force-pushed the k-60140 branch from a2373cb to 1caf1f6 Compare February 3, 2023 02:27

brianpursley force-pushed the k-60140 branch from 1caf1f6 to 728dbc7 Compare February 12, 2023 23:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 12, 2023

brianpursley force-pushed the k-60140 branch from 728dbc7 to 2f26968 Compare February 13, 2023 00:30

brianpursley changed the title ~~WIP:Disable SPDY pings for kubectl cp and add --ping flag for kubectl exec~~ Disable SPDY pings for kubectl cp and add --ping flag for kubectl exec Feb 13, 2023

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 Feb 13, 2023

aojea mentioned this pull request Feb 14, 2023

implement UserIdleTimeout moby/spdystream#86

Closed

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 23, 2023

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 30, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2023

aojea mentioned this pull request Sep 1, 2023

WebSocket Client and V5 RemoteCommand Subprotocol #119157

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2023

dims added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 24, 2023

rtheis mentioned this pull request Nov 30, 2023

Exec timeout does not kick users out on kubectl client 1.21 #102569

Closed

k8s-ci-robot closed this Feb 4, 2024

k8s-ci-robot reopened this Feb 28, 2024

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 28, 2024

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 26, 2024

k8s-ci-robot closed this Apr 25, 2024

Conversation

brianpursley commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented Feb 3, 2023

Uh oh!

brianpursley commented Feb 3, 2023

Uh oh!

brianpursley commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianpursley commented Feb 3, 2023

Uh oh!

dims commented Feb 3, 2023

Uh oh!

aojea commented Feb 4, 2023

Uh oh!

brianpursley commented Feb 12, 2023

Uh oh!

cici37 commented Feb 14, 2023

Uh oh!

brianpursley commented May 23, 2023

Uh oh!

cici37 commented May 30, 2023

Uh oh!

k8s-ci-robot commented May 30, 2023

Uh oh!

brianpursley commented May 30, 2023

Uh oh!

shnkr commented Aug 14, 2023

Uh oh!

k8s-ci-robot commented Sep 9, 2023

Uh oh!

k8s-triage-robot commented Feb 4, 2024

Uh oh!

k8s-ci-robot commented Feb 4, 2024

Uh oh!

Joseph-Goergen commented Feb 21, 2024

Uh oh!

k8s-ci-robot commented Feb 21, 2024

Uh oh!

mikebrow commented Feb 28, 2024

Uh oh!

k8s-ci-robot commented Feb 28, 2024

Uh oh!

k8s-ci-robot commented Feb 28, 2024

Uh oh!

leilajal commented Mar 26, 2024

Uh oh!

k8s-triage-robot commented Apr 25, 2024

Uh oh!

k8s-ci-robot commented Apr 25, 2024

Uh oh!

Joseph-Goergen commented Apr 29, 2024

Uh oh!

k8s-ci-robot commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

brianpursley commented Feb 3, 2023 •

edited

Loading

brianpursley commented Feb 3, 2023 •

edited

Loading