Skip to content

[release/1.6] Fix memory leak with kubectl exec >= 1.30.0#10574

Merged
mikebrow merged 7 commits intocontainerd:release/1.6from
dims:borrow-latest-wsstream-from-k8s-v1.31.x-to-1.6
Aug 13, 2024
Merged

[release/1.6] Fix memory leak with kubectl exec >= 1.30.0#10574
mikebrow merged 7 commits intocontainerd:release/1.6from
dims:borrow-latest-wsstream-from-k8s-v1.31.x-to-1.6

Conversation

@dims
Copy link
Member

@dims dims commented Aug 11, 2024 •
edited
Loading

xref: #10568
xref: kubernetes/kubernetes#126608

Fix for panic in the issues logged above are already in latest kubernetes master specifically in staged repositories. Unfortunately both 1.6 and 1.7 are on really old versions of k8s and it's not feasible/practical to move to newer versions of k8s. So we instead copy over the wsstream package which has the fix and change references in our code to the copy here.

dims added 4 commits August 10, 2024 21:00
@dims
Copy over wsstream from k8s v1.31.0-rc.1 release
a37b081 
SHA: 60c4c2b2521fb454ce69dee737e3eb91a25e0535

Signed-off-by: Davanum Srinivas <[email protected]>
@dims
Fix up some constant references
64430d6 
Signed-off-by: Davanum Srinivas <[email protected]>
@dims
switch over references to the new package
78f0799 
Signed-off-by: Davanum Srinivas <[email protected]>
@dims
Add copyright headers
a7b0c01 
Signed-off-by: Davanum Srinivas <[email protected]>
@dosubot dosubot bot added the area/cri Container Runtime Interface (CRI) label Aug 11, 2024
@dims dims changed the title [WIP] Borrow latest wsstream from k8s v1.31.x to 1.6 [release-1.6][WIP] Borrow latest wsstream from k8s v1.31.x to 1.6 Aug 11, 2024
@dims dims changed the title [release-1.6][WIP] Borrow latest wsstream from k8s v1.31.x to 1.6 [release/1.6][WIP] Borrow latest wsstream from k8s v1.31.x to 1.6 Aug 11, 2024
@dims
Run go mod tidy
05c2b14 
Signed-off-by: Davanum Srinivas <[email protected]>
@thaJeztah
Copy link
Member

thaJeztah commented Aug 12, 2024

Wondering if these should be an internal/ package (pkg/cri/internal/... ?) to prevent anyone from starting to depend on these, as this package will be gone again in v2.

@dims dims changed the title [release/1.6][WIP] Borrow latest wsstream from k8s v1.31.x to 1.6 [release/1.6] Borrow latest wsstream from k8s v1.31.x to 1.6 Aug 12, 2024
@dims
Copy link
Member Author

dims commented Aug 12, 2024

Wondering if these should be an internal/ package (pkg/cri/internal/... ?) to prevent anyone from starting to depend on these, as this package will be gone again in v2.

Done! thanks @thaJeztah

mikebrow
mikebrow reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the review of the borrow/sync PR on 1.7, code review looks good to me appears to not introduce new bad behavior :-) just need to sanity test against older k8s clients

As with 1.7 I also suppose a sanity check v1.23 min through 1.31 is needed, for 1.6, each time we change out the streaming protocol code base..

@mikebrow mikebrow self-assigned this Aug 13, 2024
@dims
Copy link
Member Author

dims commented Aug 13, 2024

Confirmed that the leak does not happen with this patch.

image

@dims
Copy link
Member Author

dims commented Aug 13, 2024

tested the following crictl versions. Note that the 1.30.0 and up has both protocols, so tested both.

1.22.1
1.23.0
1.24.0
1.24.1
1.24.2
1.25.0
1.26.0
1.26.1
1.27.0
1.27.1
1.28.0
1.29.0
1.30.0
1.30.1
1.31.0
1.31.1

@dims dims mentioned this pull request Aug 13, 2024
Merged
mikebrow
mikebrow approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow mikebrow merged commit 2958525 into containerd:release/1.6 Aug 13, 2024
@Playgirlkaybraz11 Playgirlkaybraz11 mentioned this pull request Aug 14, 2024
Closed
@samuelkarp samuelkarp changed the title [release/1.6] Borrow latest wsstream from k8s v1.31.x to 1.6 [release/1.6] Fix memory leak with kubectl exec >= 1.30.0 Sep 6, 2024
@samuelkarp samuelkarp mentioned this pull request Sep 6, 2024
Merged
@samuelkarp samuelkarp mentioned this pull request Oct 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

@mikebrow mikebrow

Labels

area/cri Container Runtime Interface (CRI) impact/changelog size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dims @thaJeztah @estesp @mikebrow @samuelkarp @k8s-ci-robot