Fix regression for timed-out stream cleanups #102489

saschagrunert · 2021-06-01T15:06:25Z

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

If a stream is already timed-out, then either the data or error stream
may be nil. This would cause a segmentation fault, which is now
covered with this patch.

Which issue(s) this PR fixes:

Fixes #102480

Special notes for your reviewer:

Has to be backported since the original PR got backported, too. :-/

Does this PR introduce a user-facing change?

Fixed a regression that can make kubelet runtime panic for timed-out portforward streams.

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

None

If a stream is already timed-out, then either the data or error stream may be `nil`. This would cause a segmentation fault, which is now covered with this patch. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert · 2021-06-01T15:14:19Z

/sig node

saschagrunert · 2021-06-01T16:52:30Z

/retest

saschagrunert · 2021-06-01T16:53:03Z

/assign @deads2k
please take a look

fedebongio · 2021-06-01T20:17:40Z

/triage accepted

wzshiming · 2021-06-02T03:14:42Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.go

+		// It may be possible that the provided stream is nil if timed out.
+		if stream != nil {
+			delete(c.streams, stream.Identifier())
+		}


I feel that adding a judgment on the caller is also a good choice

kubernetes/pkg/kubelet/cri/streaming/portforward/httpstream.go

Line 168 in eee6e54

h.conn.RemoveStreams(pair.dataStream, pair.errorStream)

Hey @wzshiming, thank you for the review! Do you mean that we should move the nil check over to the RemoveStreams() invocation in favor of checking here?

it's fine to check in both places, but I want this check to stay here, since it prevents all mistakes.

Question: will the timed out stream not remain hanging around in the c.streams map, since the identity is no longer known when calling RemoveStreams?

I guess it gets cleaned up during Close, but since the unit test below does an explicit len(c.streams) check, just wondering if something else might use a similiar check to determine whether the connection can be closed?

Yeah that's an excellent question, good reason to do reverts rather than another round of cherry-picks.

Tested it locally with a modified version of CRI-O (using this vendored code) and it looks like that the connections are getting cleaned up if the timeout got reached.

ehashman

/priority critical-urgent
/lgtm

this is why I said we should be careful about that backport 😄

dims · 2021-06-03T22:20:42Z

/assign @liggitt

liggitt · 2021-06-03T22:37:00Z

Has to be backported since the original PR got backported, too. :-/

it looks like the original got backported to 1.18 in #100954 , so the final 1.18 release is broken?

liggitt · 2021-06-03T22:52:19Z

/unassign
/assign @lavalamp

lavalamp · 2021-06-03T22:56:13Z

/lgtm
/approve

k8s-ci-robot · 2021-06-03T22:56:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, saschagrunert

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:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp]

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

lavalamp · 2021-06-03T23:00:15Z

I found the analysis here to be convincing.

lavalamp · 2021-06-03T23:02:41Z

I'm on the fence between backporting this and just reverting the cherry-picks, I was on the fence about including them in the first place. If we didn't catch this with the test, there could be other problems.

lavalamp · 2021-06-03T23:04:51Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection_test.go

@@ -323,6 +323,9 @@ func TestConnectionRemoveStreams(t *testing.T) {
 	// remove all existing
 	c.RemoveStreams(stream0, stream1)

+	// remove nil stream should not crash
+	c.RemoveStreams(nil)


This is fine, but I'd be more confident with a test that reproduces the actual crash, that way we can be confident in the diagnosis and not just the fix.

liggitt · 2021-06-03T23:13:18Z

I'm on the fence between backporting this and just reverting the cherry-picks, I was on the fence about including them in the first place. If we didn't catch this with the test, there could be other problems.

If the initial backports weren't fixing a regression, I'd vote for straight reverts. I prefer status quo from prior releases over unsoaked fixes for non-severe issues. I'm not sure what to do about the broken final 1.18 release.

fejta-bot · 2021-06-04T02:18:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

saschagrunert · 2021-06-04T07:04:09Z

I'm on the fence between backporting this and just reverting the cherry-picks, I was on the fence about including them in the first place. If we didn't catch this with the test, there could be other problems.

If the initial backports weren't fixing a regression, I'd vote for straight reverts. I prefer status quo from prior releases over unsoaked fixes for non-severe issues. I'm not sure what to do about the broken final 1.18 release.

Proposed the reverts in #102586, #102587 and #102588.

Two options for 1.18:

Leave it as-is, because it is end of life
Revert and create another patch

cc @kubernetes/sig-release-leads

lavalamp · 2021-06-04T16:07:27Z

Let's propose a revert for 1.18 and then let the release team decide what to do.

saschagrunert · 2021-06-07T07:41:19Z

Started a slack discussion, closing the loop: https://kubernetes.slack.com/archives/C2C40FMNF/p1623051662213300

Fix regression for timed-out stream cleanups

f2ca9c1

If a stream is already timed-out, then either the data or error stream may be `nil`. This would cause a segmentation fault, which is now covered with this patch. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert mentioned this pull request Jun 1, 2021

kubelet: Panic on portforward streams #102480

Closed

k8s-ci-robot requested review from ncdc and thockin June 1, 2021 15:07

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 1, 2021

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 1, 2021

k8s-ci-robot assigned deads2k Jun 1, 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 1, 2021

wzshiming reviewed Jun 2, 2021

View reviewed changes

ehashman added this to Triage in SIG Node PR Triage Jun 2, 2021

ehashman reviewed Jun 3, 2021

View reviewed changes

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 3, 2021

k8s-ci-robot assigned ehashman Jun 3, 2021

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

ehashman moved this from Triage to Needs Approver in SIG Node PR Triage Jun 3, 2021

k8s-ci-robot assigned liggitt Jun 3, 2021

k8s-ci-robot assigned lavalamp and unassigned liggitt Jun 3, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2021

lavalamp reviewed Jun 3, 2021

View reviewed changes

k8s-ci-robot merged commit b24b7d5 into kubernetes:master Jun 4, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Jun 4, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 4, 2021

saschagrunert deleted the http-stream-nil branch June 4, 2021 06:51

This was referenced Jun 4, 2021

Revert "Cleanup portforward streams after their usage" [1.20] #102586

Merged

Revert "Cleanup portforward streams after their usage" [1.21] #102587

Merged

Revert "Cleanup portforward streams after their usage" [1.19] #102588

Merged

saschagrunert mentioned this pull request Jun 14, 2021

Revert "Cleanup portforward streams after their usage" [1.18] #102838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression for timed-out stream cleanups #102489

Fix regression for timed-out stream cleanups #102489

saschagrunert commented Jun 1, 2021 •

edited by liggitt

saschagrunert commented Jun 1, 2021

saschagrunert commented Jun 1, 2021

saschagrunert commented Jun 1, 2021

fedebongio commented Jun 1, 2021

wzshiming Jun 2, 2021

saschagrunert Jun 2, 2021

lavalamp Jun 3, 2021

joshimoo Jun 4, 2021

lavalamp Jun 4, 2021

saschagrunert Jun 7, 2021

ehashman left a comment

dims commented Jun 3, 2021

liggitt commented Jun 3, 2021

liggitt commented Jun 3, 2021

lavalamp commented Jun 3, 2021

k8s-ci-robot commented Jun 3, 2021

lavalamp commented Jun 3, 2021

lavalamp commented Jun 3, 2021

lavalamp Jun 3, 2021

liggitt commented Jun 3, 2021

fejta-bot commented Jun 4, 2021

saschagrunert commented Jun 4, 2021

lavalamp commented Jun 4, 2021

saschagrunert commented Jun 7, 2021

Fix regression for timed-out stream cleanups #102489

Fix regression for timed-out stream cleanups #102489

Conversation

saschagrunert commented Jun 1, 2021 • edited by liggitt

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

saschagrunert commented Jun 1, 2021

saschagrunert commented Jun 1, 2021

saschagrunert commented Jun 1, 2021

fedebongio commented Jun 1, 2021

wzshiming Jun 2, 2021

Choose a reason for hiding this comment

saschagrunert Jun 2, 2021

Choose a reason for hiding this comment

lavalamp Jun 3, 2021

Choose a reason for hiding this comment

joshimoo Jun 4, 2021

Choose a reason for hiding this comment

lavalamp Jun 4, 2021

Choose a reason for hiding this comment

saschagrunert Jun 7, 2021

Choose a reason for hiding this comment

ehashman left a comment

Choose a reason for hiding this comment

dims commented Jun 3, 2021

liggitt commented Jun 3, 2021

liggitt commented Jun 3, 2021

lavalamp commented Jun 3, 2021

k8s-ci-robot commented Jun 3, 2021

lavalamp commented Jun 3, 2021

lavalamp commented Jun 3, 2021

lavalamp Jun 3, 2021

Choose a reason for hiding this comment

liggitt commented Jun 3, 2021

fejta-bot commented Jun 4, 2021

saschagrunert commented Jun 4, 2021

lavalamp commented Jun 4, 2021

saschagrunert commented Jun 7, 2021

saschagrunert commented Jun 1, 2021 •

edited by liggitt