Support cancelable SPDY executor stream #103177

arkbriar · 2021-06-25T07:33:13Z

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:

Sometimes I find that even simple command like "echo something" will hang there forever when there're lots of SPDY executor streams running. In the last case, I've examined all the call stacks with debug tool and still haven't figured out why. It seems all of the streams stuck on waiting for next frame and the TCP connections were still alive then. But these streams had already been existed for over 10 minutes.

Anyway, I think it will be reasonable if some bad streams can be canceled to avoid resource leaks. So I just enhance the remotecommand.Executor interface with a method accepts a context to support cancelable SPDY executor stream.

Also test have been modified to ensure it works.

Which issue(s) this PR fixes:

Fixes kubernetes/client-go#884

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a method `StreamWithContext` to remotecommand.Executor to support cancelable SPDY executor stream.

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

k8s-ci-robot · 2021-06-25T07:33:22Z

Hi @arkbriar. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

arkbriar · 2021-06-25T07:37:48Z

/assign @deads2k

fedebongio · 2021-06-29T20:07:19Z

/triage accepted

arkbriar · 2021-07-01T02:43:27Z

/assign @liggitt

staging/src/k8s.io/client-go/tools/remotecommand/remotecommand.go

aojea · 2021-07-23T23:54:30Z

should the context plumbed down to the spdy request too?
https://github.com/kubernetes/client-go/blob/ac207faedfb64acd5b99a2fb309b7044918b4dda/transport/spdy/spdy.go#L71

arkbriar · 2021-07-26T06:40:43Z

should the context plumbed down to the spdy request too?
https://github.com/kubernetes/client-go/blob/ac207faedfb64acd5b99a2fb309b7044918b4dda/transport/spdy/spdy.go#L71

@aojea For stream executor interface, it's enough to have a context here.
But yes, for the dialer interface, it still needs a context to let timeout possible.
I'll fix it in this PR. 😁

aojea · 2021-07-26T08:08:48Z

should the context plumbed down to the spdy request too?
https://github.com/kubernetes/client-go/blob/ac207faedfb64acd5b99a2fb309b7044918b4dda/transport/spdy/spdy.go#L71

@aojea For stream executor interface, it's enough to have a context here.
But yes, for the dialer interface, it still needs a context to let timeout possible.
I'll fix it in this PR.

I was trying to understand if this could also fix kubernetes/kubectl#686 , but I think I was wrong, and the spdy and Dialer context are not really needed here as you correctly pointed out.

Feel free to discard my comment since the Dialer seems to be used also in another places and changing the signature can break some external users, and this will unnecessary complicate this, sorry 😅

staging/src/k8s.io/client-go/tools/portforward/portforward.go:160:func NewOnAddresses(dialer httpstream.Dialer, addresses []string, ports []string, stopChan <-chan struct{}, readyChan chan struct{}, out, errOut io.Writer) (*PortForwarder, error) {
test/e2e/storage/drivers/proxy/portproxy.go:197:func dial(ctx context.Context, prefix string, dialer httpstream.Dialer, port int) (s *stream, finalErr error) {

/ok-to-test

Signed-off-by: arkbriar <arkbriar@gmail.com>

arkbriar · 2022-11-02T06:14:22Z

/retest-required

arkbriar · 2022-11-02T12:17:39Z

/retest-required

liggitt · 2022-11-02T12:51:33Z

/lgtm
/approve

liggitt · 2022-11-02T12:51:40Z

thanks for all the iterations on this

k8s-ci-robot · 2022-11-02T12:51:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkbriar, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/client/OWNERS~~ [liggitt]
~~pkg/kubelet/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kubectl/OWNERS~~ [liggitt]
~~test/e2e/framework/OWNERS~~ [liggitt]

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

arkbriar · 2022-11-02T15:12:15Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

aojea · 2022-11-02T22:38:09Z

/retest

failures are related to #113548

/lgtm

Thanks @arkbriar for persisting, sorry for the time it took 😄

kubernetes/kubernetes#103177

* Replace `pointer.StringPtr` with `pointer.String` * Replace `pointer.BoolPtr` with `pointer.Bool` * Replace `pointer.Int32Ptr` with `pointer.Int32` * Replace `pointer.Int64Ptr` with `pointer.Int64` * Replace deprecated `sets.String` usages appropriately * Replace `io/ioutil.ReadAll` with `io.ReadAll` As of Go 1.16, this function simply calls `io.ReadAll`. * Use `executor.StreamWithContext` instead of `executor.Stream` kubernetes/kubernetes#103177 * Use dedicated schemes for tests

golangci reported: tests/e2e/helpers/exec.go:49:9 staticcheck SA1019: exec.Stream is deprecated: use StreamWithContext instead to avoid possible resource leaks. See kubernetes/kubernetes#103177 for details. Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>

* Replace `pointer.StringPtr` with `pointer.String` * Replace `pointer.BoolPtr` with `pointer.Bool` * Replace `pointer.Int32Ptr` with `pointer.Int32` * Replace `pointer.Int64Ptr` with `pointer.Int64` * Replace deprecated `sets.String` usages appropriately * Replace `io/ioutil.ReadAll` with `io.ReadAll` As of Go 1.16, this function simply calls `io.ReadAll`. * Use `executor.StreamWithContext` instead of `executor.Stream` kubernetes/kubernetes#103177 * Use dedicated schemes for tests

reckless-huang · 2024-01-20T11:09:42Z

/复检

故障与 #113548 有关

/lgtm

感谢您的坚持，对不起，花了😄时间

greate job

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 25, 2021

k8s-ci-robot requested review from caesarxuchao and yliaog June 25, 2021 07:33

k8s-ci-robot assigned deads2k Jun 25, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 29, 2021

k8s-ci-robot assigned liggitt Jul 1, 2021

liggitt reviewed Jul 22, 2021

View reviewed changes

staging/src/k8s.io/client-go/tools/remotecommand/remotecommand.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 26, 2021

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 26, 2021

arkbriar added 2 commits November 2, 2022 11:49

use subtests and defer in TestSPDYExecutorStream

86e5d06

Signed-off-by: arkbriar <arkbriar@gmail.com>

Propagate the panic with a channel

b7e6c23

Signed-off-by: arkbriar <arkbriar@gmail.com>

arkbriar force-pushed the support_cancelable_exec_stream branch from 8dd91cf to b7e6c23 Compare November 2, 2022 03:49

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2022

k8s-ci-robot merged commit 3cf75a2 into kubernetes:master Nov 3, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 3, 2022

valaparthvi mentioned this pull request Dec 1, 2022

Ctrl-C doesn't work during 'odo dev' build phase and if build is stuck the only way out is killing the terminal redhat-developer/odo#6196

Closed

pablochacin mentioned this pull request Dec 2, 2022

xk6-disruptor-agent does not terminate if test is cancelled grafana/xk6-disruptor#82

Closed

kluevandrew mentioned this pull request Dec 19, 2022

Support cancelable SPDY executor stream tarantool/tarantool-operator#159

Closed

shafeeqes added a commit to shafeeqes/gardener that referenced this pull request Feb 7, 2023

Use executor.StreamWithContext instead of executor.Stream

23ef3bb

kubernetes/kubernetes#103177

shafeeqes added a commit to shafeeqes/gardener that referenced this pull request Feb 7, 2023

Use executor.StreamWithContext instead of executor.Stream

a8f1e6a

kubernetes/kubernetes#103177

shafeeqes added a commit to shafeeqes/gardener that referenced this pull request Feb 7, 2023

Use executor.StreamWithContext instead of executor.Stream

f8bb9c8

kubernetes/kubernetes#103177

pablochacin mentioned this pull request Feb 28, 2023

Pass context when executing command in the agent container. grafana/xk6-disruptor#116

Closed

6 tasks

piepmatz mentioned this pull request Mar 9, 2023

Support cancelable SPDY connection establishment #116412

Closed

surik mentioned this pull request Aug 22, 2023

Cleanup deprecations kubernetes-sigs/cri-tools#1250

Merged

yj-yoo mentioned this pull request Feb 21, 2024

refector: Replacing deprecated or unsafe packages kubevirt/kubevirt#11316

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cancelable SPDY executor stream #103177

Support cancelable SPDY executor stream #103177

arkbriar commented Jun 25, 2021 •

edited

k8s-ci-robot commented Jun 25, 2021

arkbriar commented Jun 25, 2021

fedebongio commented Jun 29, 2021

arkbriar commented Jul 1, 2021

aojea commented Jul 23, 2021

arkbriar commented Jul 26, 2021

aojea commented Jul 26, 2021

arkbriar commented Nov 2, 2022

arkbriar commented Nov 2, 2022

liggitt commented Nov 2, 2022

liggitt commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

arkbriar commented Nov 2, 2022

aojea commented Nov 2, 2022

reckless-huang commented Jan 20, 2024

Support cancelable SPDY executor stream #103177

Support cancelable SPDY executor stream #103177

Conversation

arkbriar commented Jun 25, 2021 • edited

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

k8s-ci-robot commented Jun 25, 2021

arkbriar commented Jun 25, 2021

fedebongio commented Jun 29, 2021

arkbriar commented Jul 1, 2021

aojea commented Jul 23, 2021

arkbriar commented Jul 26, 2021

aojea commented Jul 26, 2021

arkbriar commented Nov 2, 2022

arkbriar commented Nov 2, 2022

liggitt commented Nov 2, 2022

liggitt commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

arkbriar commented Nov 2, 2022

aojea commented Nov 2, 2022

reckless-huang commented Jan 20, 2024

arkbriar commented Jun 25, 2021 •

edited