-
Notifications
You must be signed in to change notification settings - Fork 1k
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
oci: keep track of exec PIDs and stop them on container stop #7937
Conversation
Skipping CI for Draft Pull Request. |
5bad8f6
to
0b4aacc
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7937 +/- ##
==========================================
- Coverage 48.93% 48.88% -0.06%
==========================================
Files 152 152
Lines 16452 16485 +33
==========================================
+ Hits 8051 8058 +7
- Misses 7425 7450 +25
- Partials 976 977 +1 |
36bf1b4
to
67638d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was thinking cri-o maintains a list of running execs per container somewhere, at least those with a terminals, as it has to read from those terminals (doing that in a separate goroutine I guess). Can we reuse that list?
(and yes, we don't have to kill all execs, just those with a terminal, although that's not important)
67638d4
to
fa7e972
Compare
yeah right it's in a separate goroutine, and even the context diverges between the different CRI calls, so wiring them together would be tricky. |
fa7e972
to
705513d
Compare
0932325
to
ee8e9d2
Compare
ee8e9d2
to
ac1689d
Compare
@cri-o/cri-o-maintainers this is ready, PTAL |
/retest |
1 similar comment
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, kwilczynski 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:
Approvers can indicate their approval by writing |
/cherry-pick release-1.29 |
@haircommander: new pull request created: #8072 In response to this:
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/test-infra repository. |
@haircommander, anything against cherry-picks to releases from 1.28 to 1.25? I will be happy to do it manually, if needed. |
nothing at all, sounds good! |
/cherry-pick release-1.28 |
@kwilczynski: #7937 failed to apply on top of branch "release-1.28":
In response to this:
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/test-infra repository. |
OK. Needs a manual cherry-pick. No worries. |
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on cri-o#7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on #7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on #7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Before applying this patch we killed the exec PIDs right away on container stop which leads into the failing e2e test: ``` [sig-node] [NodeFeature:SidecarContainers] Containers Lifecycle should terminate sidecars simultaneously if prestop doesn't exit ``` This regression is now fixed by killing the exec PIDs after the main container as well as in the same thread. Fixes kubernetes/kubernetes#124743 Follow-up on #7937 Needs a cherry-pick since the enhancement got already backported into supported release branches. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
What type of PR is this?
/kind bug
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?