Skip to content
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.

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

[release/1.7] Fix console TTY leak in runc shim #11250

Conversation

austinvazquez
Copy link
Member

@austinvazquez austinvazquez commented Jan 11, 2025

Backports #11161 to release/1.7 branch

The cherry-pick for 652e4d0 is not clean as some of the test utility functions are slightly different in release/1.7 and I included a skip for runc shimv1 integration testing which does not support sandbox.

@k8s-ci-robot
Copy link

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

@austinvazquez austinvazquez marked this pull request as ready for review January 11, 2025 05:19
@austinvazquez austinvazquez force-pushed the cherry-pick-aedb079bf18f1f913b705d9b791beebcf1962cdd-to-1.7 branch from f2c651e to dbb30d7 Compare January 13, 2025 16:51
@austinvazquez austinvazquez marked this pull request as draft January 13, 2025 16:51
@henry118
Copy link
Member

The tests are failing for runc v1, which does not seem to support sandbox/grouping since the service only holds a single init container. Probably we can skip this test case for runc v1 configs.

type service struct {
mu sync.Mutex
eventSendMu sync.Mutex
context context.Context
events chan interface{}
platform stdio.Platform
ec chan runcC.Exit
ep oom.Watcher
id string
container *runc.Container
cancel func()
}

@austinvazquez austinvazquez force-pushed the cherry-pick-aedb079bf18f1f913b705d9b791beebcf1962cdd-to-1.7 branch from dbb30d7 to 6335aeb Compare January 15, 2025 05:16
@austinvazquez
Copy link
Member Author

@henry118, I see good point. Let me try that.

@chrishenzie
Copy link
Contributor

Hello! Just checking if this is still blocked / if there's any updates to share?

Signed-off-by: Henry Wang <henwang@amazon.com>
(cherry picked from commit aedb079)
Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez austinvazquez force-pushed the cherry-pick-aedb079bf18f1f913b705d9b791beebcf1962cdd-to-1.7 branch from 6335aeb to 0d26a5a Compare February 11, 2025 19:16
Signed-off-by: Henry Wang <henwang@amazon.com>
(cherry picked from commit 652e4d0)
Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez austinvazquez force-pushed the cherry-pick-aedb079bf18f1f913b705d9b791beebcf1962cdd-to-1.7 branch from 0d26a5a to c3e24e0 Compare February 11, 2025 19:26
@austinvazquez
Copy link
Member Author

Sorry for the delay. Huge find by @henry118 the test was not skipping on runtime "io.containerd.runc.v1". Updated the test now.

@austinvazquez austinvazquez marked this pull request as ready for review February 11, 2025 19:49
@k8s-ci-robot
Copy link

@austinvazquez: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-containerd-node-e2e-1-7 c3e24e0 link true /test pull-containerd-node-e2e-1-7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp
Copy link
Member

estesp commented Feb 11, 2025

@dims I think this should get into our proposed 1.7 release, but seems we are having e2e test failures here (like on 1.6 as well); happy to help dig into what's going on, but not sure how to gauge whether these are flakes or a real issue we should solve before we do another 1.7.x point release.

@austinvazquez
Copy link
Member Author

@estesp , @dims, FWIW it seems the ResourceMetrics feature test failure seems to be resolved by kubernetes/kubernetes#130053. Also the Node tests failure seems to be a side-effect of the node test suite failing; not a seperate test case IIUC?

@dims
Copy link
Member

dims commented Feb 12, 2025

+1 to skip the flaky test in pull-containerd-node-e2e-1-7. let me do that now.

Copy link
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 3419f4c into containerd:release/1.7 Feb 12, 2025
57 checks passed
@austinvazquez austinvazquez deleted the cherry-pick-aedb079bf18f1f913b705d9b791beebcf1962cdd-to-1.7 branch February 12, 2025 15:31
@dmcgowan dmcgowan changed the title [release/1.7] make sure console master tty is closed on task exit [release/1.7] Fix console TTY leak in runc shim Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants