-
Notifications
You must be signed in to change notification settings - Fork 0
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
8337: add test cases for guest pull images #1
Conversation
This pr only include the test cases for guest pull with nydus-snapshotter
|
9494b5b
to
af3ee22
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.
Hey DaLi, I've made a few comments for possible ways to make the code a bit easier to read, which @wainersm might also have input on.
The other thing that we might want to consider is the reverse case of issue 8337 (unless you have it and I've missed it), where we pull without a cri-runtime handler annotation, then pull with it and verify that it's pulled on the guest properly. You seem to have the reverse, pulled with annotation, on guest and then without and (TODO) check that it isn't pulled on the guest, but maybe both ways would be useful unless you think the runc case covers that?
I also think that for test like this one that we know will fail it's fine to write them as they should work, but then add a skip, and add a note in kata-containers#8337 to unskip them in future.
Thanks for the great work!
runc_pod_config="$(new_pod_config "$unencrypted_image_1" "kata-${KATA_HYPERVISOR}")" | ||
sed -i '/runtimeClassName:/d' $runc_pod_config |
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.
This is an idea rather than direct suggestion: Rather than setting the runtime class to kata- and then deleting it, I wonder if it would be clearer to update new_pod_config
to allow it to either allow a blank runtime class, or maybe nicer create a separate to pod-config.yaml.in
that doesn't have a runtimeclass and let new_pod_config optional take the base config as the final parameter? I just wonder if that would be clearer to read and understand?
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.
On second thought (and considering some of the other suggestions I've made about extending it), maybe overloading new_pod_config
with a base runtime class is too much of an edge-case and we'd be better off just adding a comment here to say that although the config is initialised with a runtimeclass it is then immediately deleted, such that it runs as runc
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.
Thanks the comment, updated the new_pod_config
function and pod-config.yaml.in
template.
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.
Sorry, my review comments were very confusing, but I left it so that people could give their input. In my first comment I suggested making runtimeclass optional, but after thinking about the idea of making metadata_annotations
optional instead I decided that might be more useful, so suggested leaving as is with a comment. Not stuck with shell scripts and not being able to use multiple option values is an annoyance though.
Anyway these comments are mostly related to readability and re-use of code, so they can be refactored later as we add more tests and see the common patterns, so I don't want to hold up this PR due to these thoughts
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.
Added comments to say we want to get runc pod here.
|
||
# Get pod specification | ||
kubectl wait --for=condition=Ready --timeout=$timeout pod "test-e2e" | ||
set_node "$runc_pod_config" "$node" |
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.
If we need to do set_node for all our pod_configs, should we consider integrating it into new_pod_config
and pass it as a third parameter to check the test files cleaner?
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.
Part of me is wondering about adding metadata_annotations to the new_pod_config
too, but I can see scenarios, like in this file that they will need to be optional, so that would make it need to be the last parameter. Maybe still worth considering though.
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 tried to move $node
to new_pod_config
function but failed, because the runtimeClassName
is one optional parameter too one function can't have 2+ optional parameters.
I didn't make the $node
as a required parameter, I assume that we do NOT need set in for all other test cases?
So just leave the set_node
call asis
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.
@wainersm - any thoughts on this. I think you added the node call as we are running these tests on AKS now, so getting the logs requires us to know which node it's running on, so do you think it should be optional? At the moment the only usage of new_pod_config is the k8s-measured-rootfs.bats test which also calls set_node
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 moved the set_node
to new_pod_config
function, the codes looks better than before.
Both case are reversed, there are two TODO items,
|
af3ee22
to
ce076e6
Compare
@stevenhorsman @ChengyuZhu6 Many thanks for your review comments, I updated the codes, please help to take a look when you are free. |
1ad9581
to
64b8c46
Compare
Sorry, I'm not sure how I missed this last night. I guess something to think about here, and maybe it needs wider discussion is whether having TODOs and changing the expected behaviour is the correct thing to do. There is an argument that this is a pretty serious limitation, so having the tests correct and initially failing might drive useful conversations in the community and potentially get any container 2.0 fixes backported to 1.7 maybe, so we can start using this as soon as possible? I'm a bit worried that what you've done, although it passes the test, hides the problem a bit? |
Yeah, that case try to pull image on host(without annotation) first and then try to pull image on guest(with annotation)
Yeah, agree with you @stevenhorsman that current way hides the problem.
|
f7efa72
to
344aa76
Compare
So this might be kicking the can down the road a bit, but for this PR I'd recommend option 1 where we let the test case fail and then as part of the main PR 8484 (IIRC) we can test it out and have the wider discussion with the community to highlight that it is still an issue and try and get broader agreement on a path forward. |
- add test cases for guest pull images - need revist after we use container2.0 with 'image pull per runtime class' feature for kata-containers#8337 and kata-containers#8407 Signed-off-by: Da Li Liu <liudali@cn.ibm.com>
344aa76
to
0ef0ed8
Compare
Updated the test cases to use option 1 with comments provided. |
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. Thanks DaLi for the great work!
Thanks @liudalibj LGTM! |
Preparation for complete GPU rootfs build step #1/#N Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
for kata-containers#8337 and kata-containers#8407