-
Notifications
You must be signed in to change notification settings - Fork 344
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
Add ns_itype test #389
Add ns_itype test #389
Conversation
Currently I have left a few extra comments and commented code, If this is correct, I'll fix that in another commit, I have opened this PR because I wasn't sure if the current implementation is correct or not. |
Codecov Report
@@ Coverage Diff @@
## main #389 +/- ##
==========================================
- Coverage 76.59% 76.56% -0.03%
==========================================
Files 52 52
Lines 8458 8458
==========================================
- Hits 6478 6476 -2
- Misses 1980 1982 +2 |
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.
I am not clear on what do you mean by "The original test seem to do things in complecated ways". I think you have the right idea, but the namespace inode verification part of the code is not complete/comment out, so I can't be 100% sure.
You may be able to use procfs crate to easily find namespace inode. I think the original test tries to read the inode directly by looking at the /proc/
. If procfs
crate can hide these complexity for you then you are good.
The idea here is that oci runtime will bind each of the namespace in procfs. Take pid namespace for example, the namespace of the container process is bind to /proc/<pid>/ns/pid
. If the container process inherits the host namespace, then /proc/<container process pid>/ns/pid
inode should be the same as the /proc/<host pid>/ns/pid
.
youki_integration_test/src/tests/linux_ns_itype/ns_itype_test.rs
Outdated
Show resolved
Hide resolved
Actually I went over original implementation once again, and now I feel my original statement does not make sense, so ignore that part 😅
The test in current stage does the verification : instead of storing the inode separately in hashmap, it just takes the complete namespace structure of host returned by procfs (line 42), and compares that to the container's (lines 60 and 68). that way we don't have to manually compare each individual namespace separately.
Yes that's the exact reason I used procfs crate, as it does the reading of /proc/* read-link and finding inode value.
|
Super. Looking forward to the final PR :) |
Just fyi, the correct way to compare namespaces is with inode and device id, but the eq implementation of procfs namespace does that already for you, so you are fine. |
Ohh, I originally compared only by inode before changing it to direct comparison of HashMaps 😅 Thanks for the info! |
Great!! How about making sure that unwrap comes with the appropriate error message? |
Hey, @Furisto has added some great context info instead of unwraps, as well as added some helper function for dealing with the output of test_outside_container in PR #391 . I am reviewing it right now, and it seems it'd be better to merge that before this, and then I fix conflicts that will occur, as well as update. Then we can merge this PR. |
Hey, so I'll make this a draft again, incorporate changes from #391 ,and then open again for review. |
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.
LGTM. Nit: all error message should be lower case and no period at the end.
Thanks @yihuaf , fixed all capital cases, I don't think I had a period at any error end, please check now. |
Setup ns_itype test, but I'm not sure if this is set correctly or not :
The original test seem to do things in complecated ways, so I'm worried that I might have missed something.
https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_ns_itype/linux_ns_itype.go#L58 here, it loops through the namespaces, which are defined https://github.com/opencontainers/runtime-tools/blob/master/validation/util/linux_namespace.go here ; and then removes them from the default spec, https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_ns_itype/linux_ns_itype.go#L70 here. But this essentially removes all namespaces from the spec. so in my implementation, I have used the builder with empty vec to remove all namespaces. Then using procfs, I have directly compared the namespace inodes of host and container.
Is it correct? @utam0k @yihuaf can you please take a look?