8000 [release/1.6] Fix memory leak with `kubectl exec` >= 1.30.0 by dims · Pull Request #10574 · containerd/containerd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Aug 13, 2024
Merged

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

merged 7 commits into from
Aug 13, 2024

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 <davanum@gmail.com>
@dims
Fix up some constant references
64430d6 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
switch over references to the new package
78f0799 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
Add copyright headers
a7b0c01 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@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 <davanum@gmail.com>
@dims
golangci-lint should only look for problems in new code
4694b84 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@thaJeztah
Copy link
Member

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
hide wsstream under internal/ to prevent external use
6f9efd3 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@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
48 checks passed
@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
0