Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@sxllwx is is a user-visible issue? If it is, please provide a release note.

/triage accepted
/priority important-longterm
/assign

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

/test pull-kubernetes-unit

PTAL thx~. @liggitt

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

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

/lifecycle stale

Ugh, a stale bot.

/remove-lifecycle stale

A note for everyone who might be looking into this PR and trying it out with latest kind. kind is using internally containerd. Current version used there has the fix from containerd/containerd#8418 included, which means you won't get failures from e2e-s in this PR.

Newer versions of containerd, on the other hand switched to relying on k8s provided streaming implementation, this will be available in 1.8 and 2.0 versions. So to be able to get the error back, and thus verify the functionality of this PR, one needs to re-built the base image with newer containerd and use that when building a node-image from this PR (kind build node-image --base-image=...).

synced offline with @aojea and @soltysh

I still don't think plumbing up specific error details from the kubelet / containerd and skipping alerting the client that there was an error is the correct approach.

As I mentioned in #117493 (comment), the loop in the client that currently unconditionally tears down the overall streamConn when an error is seen handling a single portforward request seems like the place we should be making changes.

git diff
diff --git a/staging/src/k8s.io/client-go/tools/portforward/portforward.go b/staging/src/k8s.io/client-go/tools/portforward/portforward.go
index 83ef3e929b3..365b7dd1603 100644
--- a/staging/src/k8s.io/client-go/tools/portforward/portforward.go
+++ b/staging/src/k8s.io/client-go/tools/portforward/portforward.go
@@ -407,12 +407,22 @@ func (pf *PortForwarder) handleConnection(conn net.Conn, port ForwardedPort) {
        case <-localError:
        }
 
+       // This is from https://github.com/kubernetes/kubernetes/pull/126718
+       /*
+               reset dataStream to discard any unsent data, preventing port forwarding from being blocked.
+               we must reset dataStream before waiting on errorChan, otherwise, the blocking data will affect errorStream and cause <-errorChan to block indefinitely.
+       */
+       _ = dataStream.Reset()
+
        // always expect something on errorChan (it may be nil)
        err = <-errorChan
        if err != nil {
                runtime.HandleError(err)
-               pf.streamConn.Close()
+               // Don't tear down the whole parent port-forward pf.streamConn when there's an error handling a single request
+               // TODO: *something* has to notice and tear down the whole parent port-forward pf.streamConn when the backend is completely gone... what / where?
        }
+       // This also forces drain of the errorStream, similar to dataStream above
+       _ = errorStream.Reset()
 }
 
 // Close stops all listeners of PortForwarder.

@soltysh was going to dig into #117493 (comment) more as well

To keep everyone updated, here are the findings so far:

Adding the resets as outlined by Jordan in #117493 (comment) only partially addresses the issue. It ensures we receive all the messages, but unfortunately neither our spdy.connection nor the upstream spdy.Connection offers us option to reset the connection without actually closing it, which seems the cleanest approach atm. Without closing the channel we will hang when trying to create a new error channel in the next iteration, and will eventually timeout after 30s.
Interestingly enough, when we create our connection we also register a ping which fires every 5 seconds, sadly it reports success even when we're stuck trying to create a new stream.

(EDIT): The one test where I primarily focused on is this test from this PR, which is sending big chunks of data continuously, but from my tests will get stuck after sending ~1-2 MB of data. Surprisingly, I can keep the port-forward open and re-try sending the data after the failures and it will work again to send a similar amount of data in subsequent requests.

/hold
see #128318 and #128319 which includes changes from this PR, #126718 and additional fixes which should allow us to resolve the port-forward issue

PR needs rebase.

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-triage-robot: Closed this PR.