-
Notifications
You must be signed in to change notification settings - Fork 950
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
fix: dont set status to exit for oom condition #2772
fix: dont set status to exit for oom condition #2772
Conversation
pouch would fail to stop or remove a oom container if it was set to exited status Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2772 +/- ##
=========================================
- Coverage 69.45% 69.3% -0.16%
=========================================
Files 277 277
Lines 17429 17425 -4
=========================================
- Hits 12106 12077 -29
- Misses 3999 4016 +17
- Partials 1324 1332 +8
|
LGTM |
I think we must add a test case to cover the oom case. @zhuangqh @HusterWan |
Actually, I am wondering if we could polish the PR description.
The description above actually tells us almost nothing. Since you just repeated the error case. WDYT? @zhuangqh |
Actually, it is a horrible case that pouch cannot stop or remove a container which occurs oom. An oom container is stilling running as a normal container, what exits is the oom program. |
sure, the simplest way to reproduce this issue is writing a c program and |
It is worth adding a case to cover it via doing what you said. @zhuangqh |
Signed-off-by: zhuangqh zhuangqhc@gmail.com
Ⅰ. Describe what this PR did
fix: dont set status to exit for oom condition
pouch would fail to stop or remove a oom container if it was set
to exited status
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
CRI part using state.Error info string to identify oom status